[17.3.x]: Patch port of incremental type-check w/ import manager fix #54983
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The failures seem unrelated as |
devversion
added
action: merge
crisbeto
approved these changes
Mar 22, 2024
devversion
force-pushed
the
patch-port-import-manager
branch
from
f67e028
to
7f197fb
Compare
This commit introduces a new implementation of `ImportManager` that has numerous benefits: - It allows efficient re-use of original source file imports. * either fully re-using original imports if matching * updating existing import declarations to include new symbols. - It allows efficient re-use of previous generated imports. - The manager can be used for schematics and migrations. The implementation is a rework of the import manager that we originally built for schematics in Angular Material, but this commit improved it to be more flexible, more readable, and "correct". In follow-ups we can use this for schematics/migrations.
…manager `ImportGenerator` is the abstraction used by the translator functions to insert imports for `ExternalExpr` in an AST-agnostic way. This was built specifically for the linker which does not use any of the complex import managers- but rather re-uses `ngImport` or uses `ngImport.Bla`. This commit also switches the linker AST-agnostic generator to follow the new signatures. This was rather trivial.
This commit switches ngtsc's JS and DTS transform to use the new import manager. This is a drop-in replacement as we've updated the translator helpers in the previous commit to align with the new API suggested by the `ImportManagerV2` (to be renamed then).
…import manager Updates the type-check block generation code (also for inline type check blocks) to use the new import manager. This is now a requirement because the translator utilities from the reference emit environment expect an import manager that follows the new contract established via `ImportGenerator<TFile, TExpression>`. For type check files, we can simply print new imports as we don't expect existing imports to be updated. That is because type check files do not have any _original_ source files (or in practice— those are empty). For type check blocks inline, or constructors, imports _may_ be re-used. This is great as it helps fixing some incrementality bugs that we were seeing in the type check code. That is, sometimes the type check block code may generate imports conditionally for e.g. `TemplateRef`, or animations. Those then **prevent** incremental re-use if TCB code switches between those continously. We tried to account for that with signal inputs by always pre-generating such imports. This fixed the issue for type-check files, but for inline type check blocks this is different as we would introduce new imports in user code that would then be changed back in subsequential edit iterations. See: angular#53521 (review). In practice, the assumption was that we would be fine since user code is most likely containing imports to `@angular/core` already. That is a true assumption, but unfortunately it doesn't help with incremental re-use because TypeScript's structural change detection does not dedupe and expects 1:1 exact imports from their old source files. microsoft/TypeScript#56845 To improve incremental re-use for the type check integration, we should re-use original source file imports when possible. This commit enables this. To update imports and execute inline operations, we are now uisng `magic-string` (which is then bundled) as it simplifies the string manipulatuons.
Switches the JIT transforms to use the new import manager.
…al inputs Enables the incremental type-checking test that we never enabled when we landed signal inputs. Now that we fixed incremental re-use by re-using the existing user imports for inline type check blocks, the test is passing and can be enabled.
This commit deletes the older and now unused `ImportManager`.
To ease review and to allow for both instances to co-exist, `ImportManagerV2` was introduced. This commit renames it to `ImportManager` now that we deleted the older one.
…s/blocks This commit adds some unit tests verifying the import generation in TCB files and inline blocks. We don't seem to have any unit tests for these in general. This commit adds some, verifying some characteristics we would like to guarantee.
This commit updates the logic for preserving file overview comments to be more reliable and less dependent on previous transforms. Previously, with the old import manager, we had a utility called `addImport` that always separated import statements and non-import statements. This meant that the non-emitted statement from Tsickle for the synthetic file-overview comments no longer lived at the beginning of the file. `addImports` tried to overcome this by adding another new non-emitted statement *before* all imports. This then was later used by the transform (or was assumed!) to attach the synthetic file overview comments if the original tsickle AST Node is no longer at the top. This logic can be improved, because the import manager shouldn't need to bother about this fileoverview non-emitted statement, and the logic for re-attaching the fileoverview comment should be local. This commit fixes this and makes it a local transform.
devversion
force-pushed
the
patch-port-import-manager
branch
from
7f197fb
to
2659d77
Compare
|
This PR was merged into the repository by commit 93ce4d0. |
dylhunn
pushed a commit
that referenced
this pull request
Mar 27, 2024
…r` (#54983) This commit introduces a new implementation of `ImportManager` that has numerous benefits: - It allows efficient re-use of original source file imports. * either fully re-using original imports if matching * updating existing import declarations to include new symbols. - It allows efficient re-use of previous generated imports. - The manager can be used for schematics and migrations. The implementation is a rework of the import manager that we originally built for schematics in Angular Material, but this commit improved it to be more flexible, more readable, and "correct". In follow-ups we can use this for schematics/migrations. PR Close #54983
dylhunn
pushed a commit
that referenced
this pull request
Mar 27, 2024
…manager (#54983) `ImportGenerator` is the abstraction used by the translator functions to insert imports for `ExternalExpr` in an AST-agnostic way. This was built specifically for the linker which does not use any of the complex import managers- but rather re-uses `ngImport` or uses `ngImport.Bla`. This commit also switches the linker AST-agnostic generator to follow the new signatures. This was rather trivial. PR Close #54983
dylhunn
pushed a commit
that referenced
this pull request
Mar 27, 2024
…import manager (#54983) Updates the type-check block generation code (also for inline type check blocks) to use the new import manager. This is now a requirement because the translator utilities from the reference emit environment expect an import manager that follows the new contract established via `ImportGenerator<TFile, TExpression>`. For type check files, we can simply print new imports as we don't expect existing imports to be updated. That is because type check files do not have any _original_ source files (or in practice— those are empty). For type check blocks inline, or constructors, imports _may_ be re-used. This is great as it helps fixing some incrementality bugs that we were seeing in the type check code. That is, sometimes the type check block code may generate imports conditionally for e.g. `TemplateRef`, or animations. Those then **prevent** incremental re-use if TCB code switches between those continously. We tried to account for that with signal inputs by always pre-generating such imports. This fixed the issue for type-check files, but for inline type check blocks this is different as we would introduce new imports in user code that would then be changed back in subsequential edit iterations. See: #53521 (review). In practice, the assumption was that we would be fine since user code is most likely containing imports to `@angular/core` already. That is a true assumption, but unfortunately it doesn't help with incremental re-use because TypeScript's structural change detection does not dedupe and expects 1:1 exact imports from their old source files. microsoft/TypeScript#56845 To improve incremental re-use for the type check integration, we should re-use original source file imports when possible. This commit enables this. To update imports and execute inline operations, we are now uisng `magic-string` (which is then bundled) as it simplifies the string manipulatuons. PR Close #54983
dylhunn
pushed a commit
that referenced
this pull request
Mar 27, 2024
This commit deletes the older and now unused `ImportManager`. PR Close #54983
dylhunn
pushed a commit
that referenced
this pull request
Mar 27, 2024
) This commit updates the logic for preserving file overview comments to be more reliable and less dependent on previous transforms. Previously, with the old import manager, we had a utility called `addImport` that always separated import statements and non-import statements. This meant that the non-emitted statement from Tsickle for the synthetic file-overview comments no longer lived at the beginning of the file. `addImports` tried to overcome this by adding another new non-emitted statement *before* all imports. This then was later used by the transform (or was assumed!) to attach the synthetic file overview comments if the original tsickle AST Node is no longer at the top. This logic can be improved, because the import manager shouldn't need to bother about this fileoverview non-emitted statement, and the logic for re-attaching the fileoverview comment should be local. This commit fixes this and makes it a local transform. PR Close #54983
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Patch port of #54819