feat: add JsInfo provider to ts_config rule #315
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gregmagolan
force-pushed
the
tsconfig_jsinfo
branch
2 times, most recently
from
90c7be5 to
9a25c4d
Compare
kormide
approved these changes
Feb 11, 2023
gregmagolan
force-pushed
the
tsconfig_jsinfo
branch
from
9a25c4d to
f520185
Compare
jbedard
reviewed
Feb 11, 2023
| @@ -218,7 +218,7 @@ This is an error because Bazel does not run actions unless their outputs are nee | |||
| if len(outputs) > 0: | |||
| inputs_depset = depset( | |||
| copy_files_to_bin_actions(ctx, inputs), | |||
| transitive = transitive_inputs + [_gather_declarations_from_js_providers(ctx.attr.srcs + ctx.attr.deps)], | |||
| transitive = transitive_inputs + [_gather_declarations_from_js_providers(ctx.attr.srcs + [ctx.attr.tsconfig] + ctx.attr.deps)], | |||
There was a problem hiding this comment.
Isn't it possible there will be no tsconfig so [None] will be passed to _gather_declarations_from_js_providers? Is that ok?
There was a problem hiding this comment.
I checked the same thing. Not possible as the attribute is "tsconfig": attr.label(mandatory = True, allow_single_file = [".json"]), in the rule. You either get the user tsconfig or an auto-generated one that is done in the ts_project macro
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #271 although it looks like there is additional speed bump that
tscdoesn't follow the symlinked to the virtual store location of itsextendsso transitive npm deps of anextendscan't be resolved. I suspect this is easily reproducible outside of Bazel whenhoist = Falsein.npmrc.See comment left in
examples/linked_tsconfig_consumer/tsconfig.jsonfor more info:There are other way this could be worked around including manually propagating the dependency to downstream ts_project targets but the hacky virtual store path work-around above seems the simplest; here we just abandon the standard node_modules resolution algorithm that would incorrectly resolve to
node_modules/@lib/tsconfig/tsconfig.jsonfrom where transitive npm deps of@lib/tsconfigwould not be resolvable unless they were elsewhere on the node_modules resolution path such asnode_modules/my_transitive_dep. The downside of this work-around is that would not work outside of Bazel with the same tsconfig file since under pnpm the virtual store path would be different.@Toxaris There is a follow-up task here of reproducing the issue with tsc outside of Bazel and filing an issue to the typescript repo but I've got too many other things on my plate atm so I'm going to punt on that. If you wanted to make the repro and file an issue upstream I bet the fix is a relatively straight forward one in typescript.