Skip to content

fix(@ngtools/webpack): elide non-value aliases and remaining type-only AST constructs - #34594

Merged
alan-agius4 merged 2 commits into
angular:mainfrom
clydin:fix/elide-imports-type-edge-cases
Oct 30, 2026
Merged

alan-agius4 merged 2 commits into
angular:mainfrom
clydin:fix/elide-imports-type-edge-cases

Conversation

@clydin

@clydin clydin commented Oct 29, 2026

Copy link
Copy Markdown
Member

Follow up to #34582 to cover remaining pure-type import elision cases and
partial specifier removal in replaceBootstrap:

  • Elide imported alias symbols that do not resolve to a runtime value (or
    traverse a type-only alias declaration), handling non-type interface
    re-exports (export { Foo }) and interface parameter types under
    emitDecoratorMetadata.
  • Only rewrite a partially elided import declaration when a runtime value
    import must be removed or when the declaration was already replaced by a
    previous transformer. TypeScript skips its own import elision for
    declarations replaced by a transformer, so leaving other declarations
    untouched avoids retaining specifiers that TypeScript would have otherwise
    elided (such as inlined const enum imports).
  • Skip all non-ExpressionWithTypeArguments TypeNodes when
    emitDecoratorMetadata is disabled, and unconditionally skip
    TypeQueryNode (typeof X), ImportTypeNode,
    IndexSignatureDeclaration, ambient declare statements, and external
    export ... from '...' declarations.
  • Recursively visit child AST nodes when applying elideImports removals in
    replaceBootstrap so individual ImportSpecifiers are removed when
    platformBrowserDynamic shares an import with other used runtime exports.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request refines import elision and bootstrap replacement logic in Angular build tools to prevent over-elision of imports, particularly handling type nodes, ambient statements, and unresolved modules more robustly. The feedback highlights two key areas for improvement: optimizing AST traversal performance during node removal by targeting only import declarations, and enhancing code robustness by explicitly parenthesizing and comparing bitwise operations to avoid operator precedence issues.

Comment thread packages/ngtools/webpack/src/ivy/transformation.ts
Comment thread packages/ngtools/webpack/src/transformers/elide_imports.ts Outdated
…y AST constructs

Follow up to angular#34582 to cover remaining pure-type import elision cases and
partial specifier removal in `replaceBootstrap`:

- Elide imported alias symbols that do not resolve to a runtime value (or
  traverse a type-only alias declaration), handling non-`type` interface
  re-exports (`export { Foo }`) and interface parameter types under
  `emitDecoratorMetadata`.
- Only rewrite a partially elided import declaration when a runtime value
  import must be removed or when the declaration was already replaced by a
  previous transformer. TypeScript skips its own import elision for
  declarations replaced by a transformer, so leaving other declarations
  untouched avoids retaining specifiers that TypeScript would have otherwise
  elided (such as inlined `const enum` imports).
- Skip all non-`ExpressionWithTypeArguments` `TypeNode`s when
  `emitDecoratorMetadata` is disabled, and unconditionally skip
  `TypeQueryNode` (`typeof X`), `ImportTypeNode`,
  `IndexSignatureDeclaration`, ambient `declare` statements, and external
  `export ... from '...'` declarations.
- Recursively visit child AST nodes when applying `elideImports` removals in
  `replaceBootstrap` so individual `ImportSpecifier`s are removed when
  `platformBrowserDynamic` shares an import with other used runtime exports.
…AST constructs

Follow up to angular#34582 to apply the same import elision fixes made to
`@ngtools/webpack` to the `replaceBootstrap` transformer used by the
application builder:

- Elide imported alias symbols that do not resolve to a runtime value (or
  traverse a type-only alias declaration), handling non-`type` interface
  re-exports (`export { Foo }`) and interface parameter types under
  `emitDecoratorMetadata`.
- Only rewrite a partially elided import declaration when a runtime value
  import must be removed or when the declaration was already replaced by a
  previous transformer, so TypeScript's own import elision continues to
  apply to all other declarations.
- Skip all non-`ExpressionWithTypeArguments` `TypeNode`s when
  `emitDecoratorMetadata` is disabled, and unconditionally skip
  `TypeQueryNode` (`typeof X`), `ImportTypeNode`,
  `IndexSignatureDeclaration`, ambient `declare` statements, and external
  `export ... from '...'` declarations.
- Recursively visit child AST nodes when applying import removals in
  `replaceBootstrap` so individual `ImportSpecifier`s are removed when
  `platformBrowserDynamic` shares an import with other used runtime exports.
@clydin
clydin force-pushed the fix/elide-imports-type-edge-cases branch from 265ef0d to 19752d3 Compare October 29, 2026 21:05
@clydin clydin added the action: review The PR is still awaiting reviews from at least one requested reviewer label Oct 29, 2026
@clydin
clydin requested a review from alan-agius4 October 29, 2026 22:02
@alan-agius4 alan-agius4 added target: patch This PR is targeted for the next patch release action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Oct 30, 2026
@alan-agius4
alan-agius4 merged commit 174eb34 into angular:main Oct 30, 2026
42 checks passed
@alan-agius4

Copy link
Copy Markdown
Collaborator

This PR was merged into the repository. The changes were merged into the following branches:

@clydin
clydin deleted the fix/elide-imports-type-edge-cases branch October 30, 2026 11:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

action: merge The PR is ready for merge by the caretaker area: @angular/build area: @ngtools/webpack target: patch This PR is targeted for the next patch release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants