-
-
Notifications
You must be signed in to change notification settings - Fork 61
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: do not rely on @npm_typescript if custom tsc is provided #693
Conversation
This is a follow-up to aspect-build#675 which made the `@npm_typescript` repository less special. This replaces the (hopefully) last occurrence of this special name that cannot be changed by configuration. Alternatives I considered: - Expose the `is_typescript_5_or_greater` flag on the `ts_project` macro: Simplest option at first, but since it is only used for config validation, that would become nonsensical. - Add a `npm_typescript_package` or `npm_typescript_repository` parameter that would allow overriding all places where `@npm_typescript//` is used by default: Would offer more functionality (e.g. worker config checking), but would also be more invasive.
The primary goal is to avoid any use of Would it be possible to move any of the |
# - it would be wrong anyways | ||
# - it is only used to warn if worker support is configured incorrectly. | ||
if tsc != _tsc: | ||
is_typescript_5_or_greater = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it would be here that we can potentially set supports_worker = False
and maybe remove some code from within the rule? Does that simplify anything?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this would be correct: It is possible to supply a custom that has worker support. But if we set supports_worker = 0
here, workers would be disabled.
They way I think about this is: is_typescript_5_or_greater
is a best effort configuration fix: If we can "prove" the config is wrong, we correct it and warn. If we cannot "prove" the config is wrong, we do what it says (and it might fail).
Yes. We'd like to be able to use
I don't think this is feasible unfortunately:
One option to unmess the logic a bit would be to introduce a provider for tsc that holds the individual parts (tsc, watcher, validator, version).
|
Idea that came up in aspect-build#693
Extremely minimal outline on how a provider could look: https://github.com/aspect-build/rules_ts/pull/695/files |
Awesome, thanks for the explanations and RFC! 👍 Switching to the RFC would be a future change, and this PR is great as-is so I'm going to merge this one. |
This is a follow-up to #675 which made the
@npm_typescript
repository less special.This replaces the (hopefully) last occurrence of this special name that cannot be changed by configuration.
Alternatives I considered:
is_typescript_5_or_greater
flag on thets_project
macro: Simplest option at first, but since it is only used for config validation, that would become nonsensical.npm_typescript_package
ornpm_typescript_repository
parameter that would allow overriding all places where@npm_typescript//
is used by default: Would offer more functionality (e.g. worker config checking), but would also be more invasive.Changes are visible to end-users: yes
The change might make an incorrect warning disappear.
Test plan