refactor(migrations): support use of an ESM @angular/compiler-cli package#43657
Closed
clydin wants to merge 6 commits into
Closed
refactor(migrations): support use of an ESM @angular/compiler-cli package#43657clydin wants to merge 6 commits into
@angular/compiler-cli package#43657clydin wants to merge 6 commits into
Conversation
e0c394e to
d3830ae
Compare
devversion
approved these changes
Sep 30, 2021
devversion
left a comment
Member
There was a problem hiding this comment.
LGTM. Hopefully we can use ESM for the migrations in the future as this would simplify things a lot.
devversion
added a commit
to devversion/angular
that referenced
this pull request
Oct 1, 2021
Temporarily disables the ng_update_migrations integration test until angular#43657 lands.
dylhunn
pushed a commit
that referenced
this pull request
Oct 1, 2021
…ackage Currently, migrations and schematics must be in CommonJS format. However, framework packages will only be ESM from v13 and onward. To support this configuration, dynamic import expressions are now used to load `@angular/compiler-cli` and its new secondary entry point `@angular/compiler-cli/private/migrations`. Dynamic imports within Node.js allow the `@angular/core` migrations’ CommonJS code to load ESM code. Unfortunately, TypeScript will currently, unconditionally down-level dynamic import into a require call. `require` calls cannot load ESM code and will result in a runtime error. To workaround this, a Function constructor is used to prevent TypeScript from changing the dynamic import. Once TypeScript provides support for keeping the dynamic import this workaround can be dropped and replaced with a standard dynamic import. Due to the use of the dynamic import, a reference to the dynamically loaded modules must now be passed to all locations that use values from the modules.
The `@angular/core` migrations are currently published as CommonJS and also not bundled. Schematics, of which migrations are a type, are currently required to be CommonJS modules due to the Schematics Runtime not yet supporting ESM-based schematics. To ensure that the migration files are loaded as CommonJS modules, a nested `package.json` file has been introduced within the `schematics` directory of the package to set the `type` field for all containing JavaScript files as CommonJS. Since the migrations are also not currently bundled and the `devmode` output used to build the migrations will replace all relative imports with fully-qualified module specifiers, an additional `exports` entry must be added to allow the fully-qualified module specifiers to correctly resolve. If `devmode` did not change the relative imports the additional entry would not be needed. Bundling would also remedy the situation and is the long-term path, especially once the migrations are converted to ESM. Additional followup changes should investigate bundling each migration. The additional `exports` entry should be removed if either `devmode` behavior is changed or the migrations are bundled.
devversion
approved these changes
Oct 4, 2021
alxhub
approved these changes
Oct 4, 2021
Member
|
Caretaker: cl/400826396 is a necessary update to the g3 patch for this change. Please coordinate with me to land these together. We will also need a |
Contributor
|
This PR was merged into the repository by commit 6d1c2f7. |
dylhunn
pushed a commit
that referenced
this pull request
Oct 4, 2021
) The `@angular/core` migrations are currently published as CommonJS and also not bundled. Schematics, of which migrations are a type, are currently required to be CommonJS modules due to the Schematics Runtime not yet supporting ESM-based schematics. To ensure that the migration files are loaded as CommonJS modules, a nested `package.json` file has been introduced within the `schematics` directory of the package to set the `type` field for all containing JavaScript files as CommonJS. Since the migrations are also not currently bundled and the `devmode` output used to build the migrations will replace all relative imports with fully-qualified module specifiers, an additional `exports` entry must be added to allow the fully-qualified module specifiers to correctly resolve. If `devmode` did not change the relative imports the additional entry would not be needed. Bundling would also remedy the situation and is the long-term path, especially once the migrations are converted to ESM. Additional followup changes should investigate bundling each migration. The additional `exports` entry should be removed if either `devmode` behavior is changed or the migrations are bundled. PR Close #43657
|
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. |
This file contains hidden or 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
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
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.
Currently, migrations and schematics must be in CommonJS format. However, framework packages will only be ESM from v13 and onward. To support this configuration, dynamic import expressions are now used to load
@angular/compiler-cliand its new secondary entry point@angular/compiler-cli/private/migrations. Dynamic imports within Node.js allow the@angular/coremigrations’ CommonJS code to load ESM code. Unfortunately, TypeScript will currently, unconditionally down-level dynamic import into a require call.requirecalls cannot load ESM code and will result in a runtime error. To workaround this, a Function constructor is used to prevent TypeScript from changing the dynamic import. Once TypeScript provides support for keeping the dynamic import this workaround can be dropped and replaced with a standard dynamic import. Due to the use of the dynamic import, a reference to the dynamically loaded modules must now be passed to all locations that use values from the modules.