feat(typescript): support for declarationdir on ts_project #2048
Conversation
mistic
requested review from
alexeagle,
gregmagolan,
mattem and
soldair
as code owners
…t.bzl to include sourcemaps
mistic
force-pushed
the
declarationdir_for_ts_project
branch
from
8c7eeee to
cf8b106
Compare
| @@ -63,7 +64,9 @@ def _ts_project_impl(ctx): | |||
| if len(ctx.outputs.typings_outs) > 0: | |||
| arguments.add_all([ | |||
| "--declarationDir", | |||
| _join(ctx.bin_dir.path, ctx.label.package, ctx.attr.outdir), | |||
| ( | |||
| _join(ctx.bin_dir.path, ctx.label.package, ctx.attr.declarationdir) if ctx.attr.declarationdir else _join(ctx.bin_dir.path, ctx.label.package, ctx.attr.outdir) | |||
There was a problem hiding this comment.
oh we made a naming mistake on the others - TS has "rootDir" so we should have named the attribute "root_dir". Same with "out_dir" and "declaration_dir"
I think maybe it was because other rules have "outdir" so we wanted that one to match
It's not too late to change, none of the attributes were released yet
https://github.com/bazelbuild/rules_nodejs/blob/1.x/packages/typescript/src/internal/ts_project.bzl
Would you change all three of them in a second commit here? If you don't have time today I'll merge this and do it.
| @@ -63,7 +64,9 @@ def _ts_project_impl(ctx): | |||
| if len(ctx.outputs.typings_outs) > 0: | |||
| arguments.add_all([ | |||
| "--declarationDir", | |||
| _join(ctx.bin_dir.path, ctx.label.package, ctx.attr.outdir), | |||
| ( | |||
| _join(ctx.bin_dir.path, ctx.label.package, ctx.attr.declarationdir) if ctx.attr.declarationdir else _join(ctx.bin_dir.path, ctx.label.package, ctx.attr.outdir) | |||
There was a problem hiding this comment.
this logic would be simpler if we make declarationDir default to outDir prior to this statement
| @@ -115,7 +118,7 @@ def _ts_project_impl(ctx): | |||
| if ctx.outputs.buildinfo_out: | |||
| outputs.append(ctx.outputs.buildinfo_out) | |||
| runtime_outputs = depset(json_outs + ctx.outputs.js_outs + ctx.outputs.map_outs) | |||
| typings_outputs = ctx.outputs.typings_outs + [s for s in ctx.files.srcs if s.path.endswith(".d.ts")] | |||
| typings_outputs = ctx.outputs.typings_outs + ctx.outputs.typing_maps_outs + [s for s in ctx.files.srcs if s.path.endswith(".d.ts")] | |||
There was a problem hiding this comment.
why make this change? I don't think .map files belong in DeclarationInfo, they are meant to be consumed by editors not by other tsc actions that need to type-check
There was a problem hiding this comment.
checked with @gregmagolan - you are correct we transport .map files along with the thing they map (it's like that for .js.map files for example)
| @@ -357,6 +361,9 @@ def ts_project_macro( | |||
|
|
|||
| validate: boolean; whether to check that the tsconfig settings match the attributes. | |||
|
|
|||
| declarationdir: a string specifying a subdirectory under the bazel-out folder where generated declaration | |||
| outputs are written. | |||
There was a problem hiding this comment.
Include what is the default value pls
|
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
|
@googlebot I consent. |
|
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
alexeagle
force-pushed
the
declarationdir_for_ts_project
branch
from
f4d3c9c to
084b4d6
Compare
|
This is a BREAKING CHANGE and we should note it in release notes since the attributes renamed outdir => out_dir & rootdir => root_dir. |
…heir TypeScript names --outDir == out_dir --declarationDir == declaration_dir --rootDir == root_dir Note this is non-breaking since none of these attributes are on the 1.x branch
alexeagle
force-pushed
the
declarationdir_for_ts_project
branch
from
084b4d6 to
10706d1
Compare
|
Nope, non-breaking since none of these attributes were in 1.x |
|
Thanks both @alexeagle and @gregmagolan! Awesome work here 🎉 |
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
declarationDiroption from Typescript Compiler Options is not supported on ts_project.Issue Number: N/A
What is the new behavior?
declarationDiroption from Typescript Compiler Options is now supported on ts_project through a declarationdir param.Does this PR introduce a breaking change?
Other information