Repository navigation
fix(di): give no token to a ctor param typed by a same-file interface or type alias - #666
Conversation
There was a problem hiding this comment.
Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota. Try requesting a review at a lower effort level.
An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing. Review the Copilot code review billing settings.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
…arations once per file
- A `declare global { interface Window { .. } }` merges with lib.dom's
`declare var Window`, so ngtsc keeps `ɵɵinject(Window)`. The new check
made it `ɵɵinvalidFactory()`. Names declared in `declare global` are now
not counted as type-only for a DI token (`LocalDecls::is_type_only_token`).
- `LocalDecls` is built once per file (`StringConsts::local_decls`) instead
of once or twice per class. 2000 classes: 2.5 s -> 0.3 s.
- Move the `member_chain_root` doc back, and build a token's path only when
it is not imported.
- Tests: the value cases on a component too, an interface merged with a
namespace that holds a value, `@Inject(FOO) f: Foo`, the global interface.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bbb6dba9aa
ℹ️ 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".
…tatic
Two cases where the type-only check made a working factory invalid
(ngtsc 22.2.3 keeps the token):
- `interface Foo {}` at the top, and in a function or block
`@Injectable() class Foo { constructor(@optional() @SkipSelf() p: Foo) {} }`:
the parameter type is the class itself.
- `class NS { static Foo = class {} }` + `namespace NS { export interface Foo {} }`,
`constructor(f: NS.Foo)`: `NS.Foo` is the class's static.
`LocalDecls::is_type_only_token` now takes the class and leaves out its own
name and the members of a namespace that is also a value.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9ddad7405d
ℹ️ 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".
`interface Foo {}` + `{ var Foo = class {} }`: the `var` binds in the
module scope and merges with the interface, so ngtsc 22.2.3 keeps
`ɵɵinject(Foo)` and `type: Foo`. `LocalDecls` now records the `var`s of
blocks, `if`, loops, `try` and `switch` (not of function or class bodies).
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Fixes #653
A constructor parameter typed by an interface or type alias declared in the same file was compiled as an injection token,
i0.ɵɵinject(Foo). The interface is erased, so creating the class threwReferenceError: Foo is not defined.ngtsc 22.2.3 has no value for that type: the full output is
ɵɵinvalidFactory()and the partial output isdeps: "invalid". The class metadata here already emitted{ type: undefined }for these parameters, becauseLocalDecls::is_type_onlyinclass_metadata/builders.rscovers the localtypeToValuecases.Changes
resolve_factory_dep_namespaces(directives, pipes, injectables, NgModules) and the component'sextract_param_dependencynow askLocalDecls::is_type_onlyabout a token that is not imported. When the token is an interface or type alias declared in the file, the dep becomestype_only_invalid, the same as a type-only import (DI:import typetokens promoted to runtime value imports #288). This includes one inside a namespace (ns.Foo).LocalDeclsispub(crate)so both places use the same lookup as the metadata. It is built once per class, and only when a token is not imported.@Inject(Foo)token is left as written, as before.A name that is also a value keeps its token, as in ngtsc: an interface merged with a class, an enum.
An interface imported through a value import from another file still can't be detected one file at a time, as the issue says.
Checks
ngtsc_emit_parity_test.rs: newINVALID_DEP_CASESrows (interface, type alias, exported interface, interface in a namespace, and an interface on a directive, pipe, NgModule and component) checkɵɵinvalidFactory()anddeps:"invalid". For an@Injectable, the interface, type alias, exported interface, merged class and enum outputs match ngtsc 22.2.3. On a directive, pipe, NgModule or component, ngtsc reports NG2003 instead, the same as for the existingstring paramrows.local_value_ctor_dep_stays_the_tokenchecks the merged class and the enum.transform.rschange fails the injectable rows. Reverting thedecorator.rschange fails the component row.cargo test --all-features,cargo fmt --all -- --checkand conformance (1264/1264, snapshot unchanged) pass.Review follow-up (bbb6dba, 9ddad74, e7e1cb7)
A sweep of ~250 cases compared with ngtsc 22.2.3 (each decorator kind, full and partial) found one regression from the first commit. It is fixed here:
declare globalis not counted as type-only for a DI token (LocalDecls::is_type_only_token). One file can't see the global values it may merge with. A made-up global interface keeps its token, as before this PR (ngtsc: invalid).NS.FoowhereNSis a class merged with a namespace (class NS { static Foo = .. }+namespace NS { export interface Foo {} }).vardeclared in a block or loop ({ var Foo = class {} }, e7e1cb7). This also fixesctorParametersthere.LocalDeclsis built once per file (StringConsts::local_decls), not once or twice per class. A file with 2000 classes: 2.5 s → 0.3 s.@Inject(FOO) f: Foo, and the global interface.Still not covered (same output as before this PR, rare):
namespace Foo {})import Foo = ns.BarwhereBaris an interfaceinterface Foonext todeclare global { var Foo }ctorParameters(@Component({ jit: true })) still writestype: Foo🤖 Generated with Claude Code