Generate extra imports in local compilation mode for bundling purposes #53543

pmvald · 2023-12-13T01:32:52Z

The particular way that bundling is done in g3 requires that prod and dev sources have same or compatible import statements. Since the dev sources are compiled in local mode, then this requires local compilation to generate extra imports which imply the ones generated in full compilation mode due to resolving component's dependencies.

A new g3 specific option generateExtraImportsInLocalMode is added, which when is set and the compilation mode is local the compiler will generate extra imports.

The extra imports are generated in a way that the the prod sources can be bundled based on these extra imports plus user written imports without issue. This requires to perform static analysis as much as possible in local mode to resolve the component dependencies that are within the same target. For this to happen it is assumed that we are using local compilation mode per target, and not when per single (which is the case in g3). For external deps, we spread all external imports that could be candidate for exposing an external dependency to all the all the files in the compilation unit.

TGP when local compilation mode is enabled for dev sources plus this change: link

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/angular/angular/blob/main/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

Bugfix
Feature
Code style update (formatting, local variables)
Refactoring (no functional changes, no api changes)
Build related changes
CI related changes
Documentation content changes
angular.io application / infrastructure changes
Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

Yes
No

Other information

devversion

Leaving some comments here if we end up going with this approach. One thing I want us to consider is using the existing "aliasing" logic that is also used for full compilation. This would feel more natural and maybe avoid lots of extra code. Have you spent some time thinking about this? I feel like this would be better if we can get it to work somehow. We'd just need to resolve the scopes as much as possible (which is what are kind of doing with this PR already)

https://github.com/angular/angular/blob/main/packages/compiler-cli/src/ngtsc/imports/src/alias.ts#L60

https://github.com/angular/angular/blob/main/packages/compiler-cli/src/ngtsc/scope/src/local.ts#L540

packages/compiler-cli/src/ngtsc/translator/src/import_manager.ts

packages/compiler-cli/src/ngtsc/typecheck/src/type_check_file.ts

packages/compiler-cli/test/ngtsc/local_compilation_spec.ts

packages/compiler-cli/src/ngtsc/transform/src/compilation.ts

packages/compiler-cli/src/ngtsc/annotations/ng_module/src/handler.ts

pmvald · 2024-01-02T23:16:46Z

Leaving some comments here if we end up going with this approach. One thing I want us to consider is using the existing "aliasing" logic that is also used for full compilation. This would feel more natural and maybe avoid lots of extra code. Have you spent some time thinking about this? I feel like this would be better if we can get it to work somehow. We'd just need to resolve the scopes as much as possible (which is what are kind of doing with this PR already)

https://github.com/angular/angular/blob/main/packages/compiler-cli/src/ngtsc/imports/src/alias.ts#L60

https://github.com/angular/angular/blob/main/packages/compiler-cli/src/ngtsc/scope/src/local.ts#L540

Is this the API that generated re-exports ? We want to turn this off ideally to allow interop between local and global. I already sent you a doc for that.

devversion · 2024-01-03T07:41:14Z

This would be even better, either re-using that logic, or disabling it completely sounds even better.

packages/compiler-cli/src/main.ts

packages/compiler-cli/src/ngtsc/transform/src/compilation.ts

packages/compiler-cli/src/ngtsc/annotations/ng_module/src/handler.ts

devversion · 2024-01-18T07:42:52Z

@pmvald do we actually still need this PR if we disable extra imports in global compilation?

devversion

LGTM.

Caretaker note: No need for public API approval since this is a compiler option and the compiler is not considered as part of the public API (and it's a new option)

…ker` in the compilation workflow (#53543) This commit includes a skeleton of how the tool `LocalCompilationExtraImportsTracker` is used in the overall compilation workflow end-to-end. First of all, a new option `generateExtraImportsInLocalMode` is added, whose presence will make `LocalCompilationExtraImportsTracker` part of the compilation process. When this option is set an instance of `LocalCompilationExtraImportsTracker` is created within the NgCompiler. Then it is passed to the Ivy transformer and plumbed all the way down and the extra imports registered in it are added to the `ImportManager` instances before the imports are added from `ImportManager` to the generated file. This required adding a new method `generateSideEffectImport` to the `ImportManager`, which is an empty method and will be implemented in the subsequent commits. This commit expected to make no change in the compilation behavior as the methods are not implemented yet. PR Close #53543

As the first step, the import manager's `generateSideEffectImport` method is implemented to enable it to store info for side effect imports. Next, the helper `addImports` is modified to be able to generate correct statement for side effect imports. These changes will be tested in the subsequent commits when these tools are used to generate an actual extra import for the generated file. PR Close #53543

…able (#53543) This is to allow testing this option using bazel PR Close #53543

@NgModule

…#53543) With option `generateExtraImportsInLocalMode` set in local compilation mode, the compiler generates extra side effect imports using this rule: any external module from which an identifier is imported into an NgModule will be added as side effect import to every file in the compilation unit. To illustrate this better assume the compilation unit has source files `a.ts` and `b.ts`, and: ``` // a.ts import {SomeExternalStuff} from 'path/to/some_where'; import {SomeExternalStuff2} from 'path/to/some_where2'; ... @NgModule({imports: [SomeExternalStuff]}) ``` then the extra import `import "path/to/some_where"` will be added to both `a.js` and `b.js`. Note that this is not the case for `import "path/to/some_where2"` though, since the symbol `SomeExternalStuff2` is not imported into any NgModule. The math behind this mechanism is, in local compilation mode we cannot resolve component external dependencies fully. For example if a component in `a.ts` uses an external component defined in an external file `some_external_comp.ts` then we can generate the import to this file in `a.js`. Instead, we want to generate an import to a file that "gurantees" that `a.js` is placed after `some_external_comp.js` in the bundle. Now since the component in `some_external_comp.ts` is used in `a.ts`, then there must be a chain of imports starting from the NgModule that declares the component in `a.ts` to the component in `some_external_comp.ts`. This chain means some file in the same compilation unit as `a.ts` should import some external NgModule which includes `some_external_comp.ts` in its transitive closure and import it to some NgModule. So by adding this import to `a.js` we ensure that the bundling will have the right order. PR Close #53543

…53543) When option `generateExtraImportsInLocalMode` is set, we need to compute component local depednecies in order to generate extra imports related to them. At the same time running the register phase in general is harmless in local compilation. So we run it anyway. PR Close #53543

jessicajaniuk · 2024-01-30T15:05:48Z

This PR was merged into the repository by commit 64fa571.

…al mode when `generateExtraImportsInLocalMode` is set (#53543) In this commit the resolve method for components is run fully when the option `generateExtraImportsInLocalMode` is set. This is because we need local component depedencies in order to generate extra imports causing by them. This requires cutting some resolve phase logics that are unnecessary in local mode, such as diagnostics. PR Close #53543

…encies in local mode (#53543) With option `generateExtraImportsInLocalMode` set, in local mode the compiler generates extra imports for each component local dependencies. Here local dependencies means all component's dependencies within the same compilation unit. To achieve this, the compiler performs a "local version" of its regular static analysis to find each component's deps, and these deps are used to generate extra side effect imports. PR Close #53543

… when cycle is introduced (#53543) At the moment the extra import generation in local compilation mode fails if these extra imports produce a cycle. To handle this, the cycle handling strategy is updated for local compilation, and following the behaviour in the full compilation mode, the compiler does not generate extra import if it leads to cycle and instead leave things to the runtime. PR Close #53543

The tracker is responsible for registering the extra imports during the analysis and resolve compiler phases, and later to be used by the transformer to get a list of extra imports to be generated for each source file. This commit only contains the API, and the actual implementation for each method will be done in subsequent commits where an application of that method is available and so tests can be written for the implementation. PR Close angular#53543

…ker` in the compilation workflow (angular#53543) This commit includes a skeleton of how the tool `LocalCompilationExtraImportsTracker` is used in the overall compilation workflow end-to-end. First of all, a new option `generateExtraImportsInLocalMode` is added, whose presence will make `LocalCompilationExtraImportsTracker` part of the compilation process. When this option is set an instance of `LocalCompilationExtraImportsTracker` is created within the NgCompiler. Then it is passed to the Ivy transformer and plumbed all the way down and the extra imports registered in it are added to the `ImportManager` instances before the imports are added from `ImportManager` to the generated file. This required adding a new method `generateSideEffectImport` to the `ImportManager`, which is an empty method and will be implemented in the subsequent commits. This commit expected to make no change in the compilation behavior as the methods are not implemented yet. PR Close angular#53543

…lar#53543) As the first step, the import manager's `generateSideEffectImport` method is implemented to enable it to store info for side effect imports. Next, the helper `addImports` is modified to be able to generate correct statement for side effect imports. These changes will be tested in the subsequent commits when these tools are used to generate an actual extra import for the generated file. PR Close angular#53543

…able (angular#53543) This is to allow testing this option using bazel PR Close angular#53543

@NgModule

…angular#53543) With option `generateExtraImportsInLocalMode` set in local compilation mode, the compiler generates extra side effect imports using this rule: any external module from which an identifier is imported into an NgModule will be added as side effect import to every file in the compilation unit. To illustrate this better assume the compilation unit has source files `a.ts` and `b.ts`, and: ``` // a.ts import {SomeExternalStuff} from 'path/to/some_where'; import {SomeExternalStuff2} from 'path/to/some_where2'; ... @NgModule({imports: [SomeExternalStuff]}) ``` then the extra import `import "path/to/some_where"` will be added to both `a.js` and `b.js`. Note that this is not the case for `import "path/to/some_where2"` though, since the symbol `SomeExternalStuff2` is not imported into any NgModule. The math behind this mechanism is, in local compilation mode we cannot resolve component external dependencies fully. For example if a component in `a.ts` uses an external component defined in an external file `some_external_comp.ts` then we can generate the import to this file in `a.js`. Instead, we want to generate an import to a file that "gurantees" that `a.js` is placed after `some_external_comp.js` in the bundle. Now since the component in `some_external_comp.ts` is used in `a.ts`, then there must be a chain of imports starting from the NgModule that declares the component in `a.ts` to the component in `some_external_comp.ts`. This chain means some file in the same compilation unit as `a.ts` should import some external NgModule which includes `some_external_comp.ts` in its transitive closure and import it to some NgModule. So by adding this import to `a.js` we ensure that the bundling will have the right order. PR Close angular#53543

…ngular#53543) When option `generateExtraImportsInLocalMode` is set, we need to compute component local depednecies in order to generate extra imports related to them. At the same time running the register phase in general is harmless in local compilation. So we run it anyway. PR Close angular#53543

…al mode when `generateExtraImportsInLocalMode` is set (angular#53543) In this commit the resolve method for components is run fully when the option `generateExtraImportsInLocalMode` is set. This is because we need local component depedencies in order to generate extra imports causing by them. This requires cutting some resolve phase logics that are unnecessary in local mode, such as diagnostics. PR Close angular#53543

…encies in local mode (angular#53543) With option `generateExtraImportsInLocalMode` set, in local mode the compiler generates extra imports for each component local dependencies. Here local dependencies means all component's dependencies within the same compilation unit. To achieve this, the compiler performs a "local version" of its regular static analysis to find each component's deps, and these deps are used to generate extra side effect imports. PR Close angular#53543

… when cycle is introduced (angular#53543) At the moment the extra import generation in local compilation mode fails if these extra imports produce a cycle. To handle this, the cycle handling strategy is updated for local compilation, and following the behaviour in the full compilation mode, the compiler does not generate extra import if it leads to cycle and instead leave things to the runtime. PR Close angular#53543

angular-automatic-lock-bot · 2024-03-01T00:10:04Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

pmvald force-pushed the lcm-extra-imports2 branch from 6f13197 to 4350666 Compare December 13, 2023 19:33

angular-robot bot added the detected: feature PR contains a feature commit label Dec 13, 2023

pmvald force-pushed the lcm-extra-imports2 branch 4 times, most recently from 1791630 to c7b20e7 Compare December 14, 2023 00:37

pmvald changed the title ~~Lcm extra imports2~~ Generate extra imports in local compilation mode for bundling purposes Dec 14, 2023

pmvald added action: review The PR is still awaiting reviews from at least one requested reviewer area: compiler Issues related to `ngc`, Angular's template compiler PullApprove: disable labels Dec 14, 2023

ngbot bot added this to the Backlog milestone Dec 14, 2023

pmvald added the target: minor This PR is targeted for the next minor release label Dec 14, 2023

pmvald marked this pull request as ready for review December 14, 2023 03:33

pmvald requested review from devversion, crisbeto and AndrewKushnir December 14, 2023 03:33

AndrewKushnir requested a review from alxhub December 16, 2023 00:39

devversion reviewed Jan 2, 2024

View reviewed changes

pmvald force-pushed the lcm-extra-imports2 branch from c7b20e7 to 1492e71 Compare January 2, 2024 23:13

pmvald force-pushed the lcm-extra-imports2 branch 2 times, most recently from 8f802bf to 5faa37d Compare January 3, 2024 19:57

AndrewKushnir mentioned this pull request Jan 5, 2024

[Test-only] Local compilation mode changes #53802

Closed

alxhub requested changes Jan 10, 2024

View reviewed changes

packages/compiler-cli/src/main.ts Outdated Show resolved Hide resolved

packages/compiler-cli/src/ngtsc/transform/src/compilation.ts Outdated Show resolved Hide resolved

packages/compiler-cli/src/ngtsc/annotations/ng_module/src/handler.ts Outdated Show resolved Hide resolved

pmvald force-pushed the lcm-extra-imports2 branch 4 times, most recently from 3ecb305 to 401354d Compare January 18, 2024 07:12

pmvald requested a review from devversion January 30, 2024 03:11

devversion approved these changes Jan 30, 2024

View reviewed changes

devversion added action: merge The PR is ready for merge by the caretaker PullApprove: disable and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Jan 30, 2024

devversion removed request for atscott, dylhunn and jessicajaniuk January 30, 2024 13:59

jessicajaniuk closed this in ac3c484 Jan 30, 2024

jessicajaniuk pushed a commit that referenced this pull request Jan 30, 2024

refactor(bazel): make option generateExtraImportsInLocalMode configur…

5feed80

…able (#53543) This is to allow testing this option using bazel PR Close #53543

nikvarma pushed a commit to nikvarma/angular that referenced this pull request Jan 31, 2024

refactor(bazel): make option generateExtraImportsInLocalMode configur…

133b49e

…able (angular#53543) This is to allow testing this option using bazel PR Close angular#53543

angular-automatic-lock-bot bot locked and limited conversation to collaborators Mar 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generate extra imports in local compilation mode for bundling purposes #53543

Generate extra imports in local compilation mode for bundling purposes #53543

pmvald commented Dec 13, 2023 •

edited

devversion left a comment •

edited

pmvald commented Jan 2, 2024

devversion commented Jan 3, 2024

devversion commented Jan 18, 2024

devversion left a comment

jessicajaniuk commented Jan 30, 2024

angular-automatic-lock-bot bot commented Mar 1, 2024

Generate extra imports in local compilation mode for bundling purposes #53543

Generate extra imports in local compilation mode for bundling purposes #53543

Conversation

pmvald commented Dec 13, 2023 • edited

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

devversion left a comment • edited

Choose a reason for hiding this comment

pmvald commented Jan 2, 2024

devversion commented Jan 3, 2024

devversion commented Jan 18, 2024

devversion left a comment

Choose a reason for hiding this comment

jessicajaniuk commented Jan 30, 2024

angular-automatic-lock-bot bot commented Mar 1, 2024

pmvald commented Dec 13, 2023 •

edited

devversion left a comment •

edited