Conversation
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. |
Co-authored-by: Dunqing <29533304+Dunqing@users.noreply.github.com>
Changed namespace reference check from has_value_reference() to has_reference() to include both type and value references. This ensures that namespaces referenced in type positions (e.g., Foo.Bar in type aliases) are included in the output. Also fixes the shadowed.ts test case where Module namespace is now correctly included when referenced in a type. Co-authored-by: Dunqing <29533304+Dunqing@users.noreply.github.com>
Add test cases covering: - Simple namespace reference - Nested namespace reference - Multiple references to same namespace - Namespace in union types - Namespace in interfaces - Unreferenced namespace (should be excluded) Co-authored-by: Dunqing <29533304+Dunqing@users.noreply.github.com>
CodSpeed Performance ReportMerging #15123 will not alter performanceComparing Summary
Footnotes
|
There was a problem hiding this comment.
Pull Request Overview
This PR fixes an issue where TypeScript namespaces were incorrectly excluded from generated declaration files when they were only referenced in type positions (e.g., Foo.Bar in type aliases). The change ensures namespaces are included when referenced as either types or values.
Key changes:
- Updated namespace reference checking from value-only to all references (type and value)
- Added test cases for namespace type references in various scenarios
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| crates/oxc_isolated_declarations/src/declaration.rs | Changed namespace reference check from has_value_reference to has_reference to include both type and value references |
| crates/oxc_isolated_declarations/tests/snapshots/shadowed.snap | Updated snapshot showing namespace now correctly included when referenced in type position |
| crates/oxc_isolated_declarations/tests/snapshots/namespace-type-reference.snap | New test snapshot for simple namespace type reference scenario |
| crates/oxc_isolated_declarations/tests/snapshots/namespace-type-reference-complex.snap | New test snapshot for complex namespace type reference scenarios |
| crates/oxc_isolated_declarations/tests/fixtures/namespace-type-reference.ts | New test fixture for basic namespace type reference |
| crates/oxc_isolated_declarations/tests/fixtures/namespace-type-reference-complex.ts | New test fixture covering various namespace reference patterns |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
When an exported type references a namespaced type (e.g.,
export type A = Foo.Bar), the namespace declaration was being dropped from the output, causing the type reference to resolve toany.Changes
declaration.rs: Changed namespace inclusion check fromhas_value_reference()tohas_reference()to account for type-position references. This aligns with enum handling, which also useshas_reference()since both can serve dual roles as types and values.Test coverage: Added test cases for simple, nested, and multiple namespace references in various type positions (aliases, unions, interfaces).
Technical Details
Namespaces are bound with
KindFlags::All(both type and value), but the transform logic was only checking for value references. Type references likeFoo.Barare tracked asKindFlags::Typethroughvisit_ts_type_name(), so the previous check excluded legitimately referenced namespaces.Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.