fix: support npm_typescript definitions from multiple modules#884
fix: support npm_typescript definitions from multiple modules#884jbedard merged 2 commits intoaspect-build:mainfrom
Conversation
|
26dad8d to
e6b7968
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c3c2727d3b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| selected = {} | ||
| for mod in module_ctx.modules: | ||
| is_root = hasattr(mod, "is_root") and mod.is_root | ||
| for attr in mod.tags.deps: | ||
| if attr.ts_version_from and hasattr(module_ctx, "watch"): | ||
| module_ctx.watch(attr.ts_version_from) | ||
| existing = selected.get(attr.name) | ||
| if existing and not is_root: | ||
| continue |
There was a problem hiding this comment.
Reject conflicting npm_typescript versions
When multiple non-root modules register the same name but point at different ts_version/ts_version_from, the new de‑duplication silently keeps the first module in module_ctx.modules and drops the rest. In that scenario the dropped module is compiled with the wrong TypeScript compiler (whatever happened to be selected), which can change diagnostics or emitted JS. This only happens when dependency modules specify different TS versions and there is no root override. Previously this would error due to duplicate repo names; now it succeeds with a potentially incorrect toolchain. Consider validating that duplicate name entries are identical (or explicitly error) before picking one.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@dzbarsky wdyt about this? I wonder if we should fail() if there are multiple non-root versions that differ? In which case it will fail() and the user must:
- convince one of those non-root modules to use a unique
name - explicitly declare a
ts_versionin the root to override those non-roots
There was a problem hiding this comment.
See followup commit for an example of that...
Fix #843
Changes are visible to end-users: no
Test plan