fix(typescript): add json to ts_project DefaultInfo, fix #1988 #1995
Conversation
longlho
requested review from
alexeagle,
gregmagolan,
mattem and
soldair
as code owners
| load("@build_bazel_rules_nodejs//:index.bzl", "generated_file_test") | ||
| load("//packages/typescript:index.bzl", "ts_project") | ||
|
|
||
| SRCS = glob(["**/*.ts*"]) + [ |
There was a problem hiding this comment.
aside: interesting pattern rather than ["**/*.ts", "**/*.tsx"] - I just wish the glob pattern allowed ["**/*.ts?"] so this wouldn't accidentally overmatch. But not much starts with "ts" so I'm considering adopting this in react projects...
|
|
||
| json_outs = ctx.outputs.json_outs | ||
|
|
||
| # If there are no predeclared json_outs (probably bc of no .json or no outdir), |
There was a problem hiding this comment.
this comment leaves an unanswered question: if there are no .json srcs and no .outdir then why do we need to declare any json_outs?
Is this load-bearing in some way? I pulled the change and your new test passes without this, so either remove it or make the test show why it's needed and update the comment.
There was a problem hiding this comment.
so the predeclared json_outs outputs are only computed when both outdir is there as an attr & srcs contains json.
- If there's no
json, there's nothing to do. - If there're some
jsons butoutdirisn't there,tscbasically does acopy_to_bin-like behavior which is why the test passes for theno-outdircase (it grabs the srcjsoninstead of the copied one to compare?)
There was a problem hiding this comment.
I've updated the test that should demo this :)
There was a problem hiding this comment.
it still feels like this code is in the wrong place, compensating for json_outs having been passed into this function with the wrong value. Can it be fixed at the callsite instead? (line 402)
Seems like it just needs to match how js_outs is computed and passed in?
There was a problem hiding this comment.
initially I was doing just _join(outdir, f for f in srcs if f.endswith(".json")) but if outdir is None then I got input & output are the same. This is what special about json input :( For .ts u always get a .js output file but for json it's the same file if outdir isn't specified. Any rec on how to fix this?
There was a problem hiding this comment.
Right, makes sense, it's like copy_to_bin so you can't pre declare labels for outputs. What if we never declare them? (Degrade to common case for when outdir specified)?
There was a problem hiding this comment.
Yeah I tried just declare_file instead of splitting btwn predeclared outputs & declare_file but I can't get the path concat right for outdir, the file path in ctx.files.srcs seems to be the full path so I need some path mangling logic that I'm not sure if my assumptions are correct. Any guidance there?
There was a problem hiding this comment.
ah, I see, yes at this point the File object doesn't give you the path of the input relative to the package so you have to calculate it yourself
src.dirname[len(ctx.label.package)+1:]
I should have a commit to add here shortly...
There was a problem hiding this comment.
awesome thanks a lot!
| @@ -389,6 +397,9 @@ def ts_project_macro( | |||
| tsconfig = tsconfig, | |||
| extends = extends, | |||
| outdir = outdir, | |||
| # JSON files are special. They are output if outdir is set since tsc will | |||
| # copy them to outdir. Otherwise they are kind of both. | |||
There was a problem hiding this comment.
good explanation 👍 if tsc copies them to outdir then they are outputs of the action, easy justification
Never predeclare these outputs, since in most cases we can't
|
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. |
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?
Issue Number: #1988
What is the new behavior?
Does this PR introduce a breaking change?
Other information