Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Various standalone migrations improvements #48987

Closed

Conversation

crisbeto
Copy link
Member

@crisbeto crisbeto commented Feb 7, 2023

Includes the following improvements to the standalone migration. There is more info in each of the commits:

  1. Only excludes bootstrapped declarations from the first step of the migration, rather than all of them.
  2. Migrates tests when switching the root module to standalone.
  3. Doesn't copy over ModuleWithProviders when migrating tests.
  4. Preserves the project's tsconfig when constructing the migration program.
  5. Reduces the number of files that the migration has to load in order to speed it up.
  6. Corrects any standalone declarations that might still be left in the declarations array from a previous migration. This is primarily targeted towards tests.

Fixes #48944.
Fixes #48971.

@crisbeto crisbeto added action: review The PR is still awaiting reviews from at least one requested reviewer area: migrations Issues related to `ng update` migrations target: minor This PR is targeted for the next minor release labels Feb 7, 2023
@ngbot ngbot bot modified the milestone: Backlog Feb 7, 2023
@crisbeto crisbeto requested a review from dylhunn February 7, 2023 13:33
Copy link
Contributor

@dylhunn dylhunn left a comment

Choose a reason for hiding this comment

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

Looks good, and seems to fix the mentioned issue reports. (It's pretty involved though, so it's hard for me to be totally sure there are no new bugs.)


// If we can't analyze the `bootstrap` property, we can't safely migrate the module.
if (!ts.isPropertyAssignment(bootstrapProp) ||
!ts.isArrayLiteralExpression(bootstrapProp.initializer)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the purpose of excluding array literals, since we already know the property is bootstrap?

Copy link
Member Author

Choose a reason for hiding this comment

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

We're excluding anything that isn't an array literal, because we can't easily analyze it. E.g. bootstrap: SOME_CONST.

…standalone migration

Currently the standalone migration is set up to skip any modules that have a `bootstrap` array with at least one element. This ends up being misleading for small apps who have everything in the root module.

These changes add some logic to only skip the root component.

Fixes angular#48944.
… API

The first step of the migration updates the tests of the component being migrated, however it also skips any bootstraped declarations. We get back to the these declarations when converting to the standalone bootstrapping APIs, but we weren't updating their tests.

These changes rework some of the logic so that we migrate any remaining tests as a part of the final step.

Relates to angular#48944.
…ponents

Fixes that we were copying all expressions in the `imports` array of the test to the `imports` of the component, including any potential `ModuleWithProviders`.

Fixes angular#48971.
For the standalone migration we need to pass a couple of compiler flags which accidentally also overwrote the project's compiler options. These changes extend the options instead.
Attempts to speed up the standalone migration by:
1. Not analyzing typechecker files.
2. Telling TS not to check libraries.
@crisbeto crisbeto force-pushed the bootstrapped-module-improvements branch from ea0aa1b to e30fb30 Compare February 8, 2023 07:58
Normally having a standalone declaration in the `imports` array is an error and something we handle in the conversion to standalone, but tests can end up in this situation, because apps may have separate tsconfigs for the main app and for tests.

These changes make it so that we move any incorrectly-defined standalone declarations, even if they aren't part of the current migration.
@crisbeto
Copy link
Member Author

crisbeto commented Feb 8, 2023

I pushed one extra commit to handle any leftover invalid tests.

@crisbeto crisbeto added 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 Feb 8, 2023
@crisbeto crisbeto removed the request for review from devversion February 8, 2023 09:52
@github-actions
Copy link

github-actions bot commented Feb 8, 2023

Deployed aio for 5e00e34 to: https://ng-dev-previews-fw--pr-angular-angular-48987-h2o9o0et.web.app

Note: As new commits are pushed to this pull request, this link is updated after the preview is rebuilt.

@pkozlowski-opensource
Copy link
Member

This PR was merged into the repository by commit c7926b5.

pkozlowski-opensource pushed a commit that referenced this pull request Feb 8, 2023
… API (#48987)

The first step of the migration updates the tests of the component being migrated, however it also skips any bootstraped declarations. We get back to the these declarations when converting to the standalone bootstrapping APIs, but we weren't updating their tests.

These changes rework some of the logic so that we migrate any remaining tests as a part of the final step.

Relates to #48944.

PR Close #48987
pkozlowski-opensource pushed a commit that referenced this pull request Feb 8, 2023
…ponents (#48987)

Fixes that we were copying all expressions in the `imports` array of the test to the `imports` of the component, including any potential `ModuleWithProviders`.

Fixes #48971.

PR Close #48987
pkozlowski-opensource pushed a commit that referenced this pull request Feb 8, 2023
For the standalone migration we need to pass a couple of compiler flags which accidentally also overwrote the project's compiler options. These changes extend the options instead.

PR Close #48987
pkozlowski-opensource pushed a commit that referenced this pull request Feb 8, 2023
Attempts to speed up the standalone migration by:
1. Not analyzing typechecker files.
2. Telling TS not to check libraries.

PR Close #48987
pkozlowski-opensource pushed a commit that referenced this pull request Feb 8, 2023
Normally having a standalone declaration in the `imports` array is an error and something we handle in the conversion to standalone, but tests can end up in this situation, because apps may have separate tsconfigs for the main app and for tests.

These changes make it so that we move any incorrectly-defined standalone declarations, even if they aren't part of the current migration.

PR Close #48987
crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this pull request Feb 23, 2023
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [@angular/animations](https://github.com/angular/angular) | dependencies | minor | [`15.1.5` -> `15.2.0`](https://renovatebot.com/diffs/npm/@angular%2fanimations/15.1.5/15.2.0) |
| [@angular/common](https://github.com/angular/angular) | dependencies | minor | [`15.1.5` -> `15.2.0`](https://renovatebot.com/diffs/npm/@angular%2fcommon/15.1.5/15.2.0) |
| [@angular/compiler](https://github.com/angular/angular) | dependencies | minor | [`15.1.5` -> `15.2.0`](https://renovatebot.com/diffs/npm/@angular%2fcompiler/15.1.5/15.2.0) |
| [@angular/compiler-cli](https://github.com/angular/angular/tree/main/packages/compiler-cli) ([source](https://github.com/angular/angular)) | devDependencies | minor | [`15.1.5` -> `15.2.0`](https://renovatebot.com/diffs/npm/@angular%2fcompiler-cli/15.1.5/15.2.0) |
| [@angular/core](https://github.com/angular/angular) | dependencies | minor | [`15.1.5` -> `15.2.0`](https://renovatebot.com/diffs/npm/@angular%2fcore/15.1.5/15.2.0) |
| [@angular/forms](https://github.com/angular/angular) | dependencies | minor | [`15.1.5` -> `15.2.0`](https://renovatebot.com/diffs/npm/@angular%2fforms/15.1.5/15.2.0) |
| [@angular/platform-browser](https://github.com/angular/angular) | dependencies | minor | [`15.1.5` -> `15.2.0`](https://renovatebot.com/diffs/npm/@angular%2fplatform-browser/15.1.5/15.2.0) |
| [@angular/platform-browser-dynamic](https://github.com/angular/angular) | dependencies | minor | [`15.1.5` -> `15.2.0`](https://renovatebot.com/diffs/npm/@angular%2fplatform-browser-dynamic/15.1.5/15.2.0) |

---

### Release Notes

<details>
<summary>angular/angular</summary>

### [`v15.2.0`](https://github.com/angular/angular/blob/HEAD/CHANGELOG.md#&#8203;1520-2023-02-22)

[Compare Source](angular/angular@15.1.5...15.2.0)

#### Deprecations

#####

-   Class and `InjectionToken` guards and resolvers are
    deprecated. Instead, write guards as plain JavaScript functions and
    inject dependencies with `inject` from `@angular/core`.

#####

| Commit | Type | Description |
| -- | -- | -- |
| [926c35f4ac](angular/angular@926c35f) | docs | Deprecate class and InjectionToken and resolvers ([#&#8203;47924](angular/angular#47924)) |

##### common

| Commit | Type | Description |
| -- | -- | -- |
| [54b24eb40f](angular/angular@54b24eb) | feat | Add loaderParams attribute to NgOptimizedImage ([#&#8203;48907](angular/angular#48907)) |

##### compiler-cli

| Commit | Type | Description |
| -- | -- | -- |
| [0cf11167f1](angular/angular@0cf1116) | fix | incorrectly detecting forward refs when symbol already exists in file ([#&#8203;48988](angular/angular#48988)) |

##### core

| Commit | Type | Description |
| -- | -- | -- |
| [a154db8a81](angular/angular@a154db8) | feat | add ng generate schematic to convert declarations to standalone ([#&#8203;48790](angular/angular#48790)) |
| [345e737daa](angular/angular@345e737) | feat | add ng generate schematic to convert to standalone bootstrapping APIs ([#&#8203;48848](angular/angular#48848)) |
| [e7318fc758](angular/angular@e7318fc) | feat | add ng generate schematic to remove unnecessary modules ([#&#8203;48832](angular/angular#48832)) |

##### language-service

| Commit | Type | Description |
| -- | -- | -- |
| [4ae384fd61](angular/angular@4ae384f) | feat | Allow auto-imports of a pipe via quick fix when its selector is used, both directly and via reexports. ([#&#8203;48354](angular/angular#48354)) |
| [141333411e](angular/angular@1413334) | feat | Introduce a new NgModuleIndex, and use it to suggest re-exports. ([#&#8203;48354](angular/angular#48354)) |
| [d0145033bd](angular/angular@d014503) | fix | generate forwardRef for same file imports ([#&#8203;48898](angular/angular#48898)) |

##### migrations

| Commit | Type | Description |
| -- | -- | -- |
| [2796230e95](angular/angular@2796230) | fix | add `enum` in `mode` option in `standalone` schema ([#&#8203;48851](angular/angular#48851)) |
| [816e76a578](angular/angular@816e76a) | fix | automatically prune root module after bootstrap step ([#&#8203;49030](angular/angular#49030)) |
| [bdbf21d04b](angular/angular@bdbf21d) | fix | avoid generating imports with forward slashes ([#&#8203;48993](angular/angular#48993)) |
| [32cf4e5cb9](angular/angular@32cf4e5) | fix | avoid internal modules when generating imports ([#&#8203;48958](angular/angular#48958)) |
| [521ccfbe6c](angular/angular@521ccfb) | fix | avoid interrupting the migration if language service lookup fails ([#&#8203;49010](angular/angular#49010)) |
| [a40cd47aa7](angular/angular@a40cd47) | fix | avoid modifying testing modules without declarations ([#&#8203;48921](angular/angular#48921)) |
| [1afa6ed322](angular/angular@1afa6ed) | fix | don't add ModuleWithProviders to standalone test components ([#&#8203;48987](angular/angular#48987)) |
| [c98c6a8452](angular/angular@c98c6a8) | fix | don't copy animations modules into the imports of test components ([#&#8203;49147](angular/angular#49147)) |
| [8389557848](angular/angular@8389557) | fix | don't copy unmigrated declarations into imports array ([#&#8203;48882](angular/angular#48882)) |
| [f82bdc4b01](angular/angular@f82bdc4) | fix | don't delete classes that may provide dependencies transitively ([#&#8203;48866](angular/angular#48866)) |
| [759db12e0b](angular/angular@759db12) | fix | duplicated comments on migrated classes ([#&#8203;48966](angular/angular#48966)) |
| [ba38178d19](angular/angular@ba38178) | fix | generate forwardRef for same file imports ([#&#8203;48898](angular/angular#48898)) |
| [03fcb36cfd](angular/angular@03fcb36) | fix | migrate HttpClientModule to provideHttpClient() ([#&#8203;48949](angular/angular#48949)) |
| [2de6dae16d](angular/angular@2de6dae) | fix | migrate RouterModule.forRoot with a config object to use features ([#&#8203;48935](angular/angular#48935)) |
| [770191cf1f](angular/angular@770191c) | fix | migrate tests when switching to standalone bootstrap API ([#&#8203;48987](angular/angular#48987)) |
| [c7926b5773](angular/angular@c7926b5) | fix | move standalone migrations into imports ([#&#8203;48987](angular/angular#48987)) |
| [65c74ed93e](angular/angular@65c74ed) | fix | normalize paths to posix ([#&#8203;48850](angular/angular#48850)) |
| [6377487b1a](angular/angular@6377487) | fix | only exclude bootstrapped declarations from initial standalone migration ([#&#8203;48987](angular/angular#48987)) |
| [e9e4449a43](angular/angular@e9e4449) | fix | preserve tsconfig in standalone migration ([#&#8203;48987](angular/angular#48987)) |
| [ffad1b49d9](angular/angular@ffad1b4) | fix | reduce number of files that need to be checked ([#&#8203;48987](angular/angular#48987)) |
| [ba7a757cc5](angular/angular@ba7a757) | fix | return correct alias when conflicting import exists ([#&#8203;49139](angular/angular#49139)) |
| [49a7c9f94a](angular/angular@49a7c9f) | fix | standalone migration incorrectly throwing path error for multi app projects ([#&#8203;48958](angular/angular#48958)) |
| [584976e6c8](angular/angular@584976e) | fix | support --defaults in standalone migration ([#&#8203;48921](angular/angular#48921)) |
| [03f47ac901](angular/angular@03f47ac) | fix | use consistent quotes in generated imports ([#&#8203;48876](angular/angular#48876)) |
| [ebae506d89](angular/angular@ebae506) | fix | use import remapper in root component ([#&#8203;49046](angular/angular#49046)) |
| [40c976c909](angular/angular@40c976c) | fix | use NgForOf instead of NgFor ([#&#8203;49022](angular/angular#49022)) |
| [4ac25b2aff](angular/angular@4ac25b2) | perf | avoid re-traversing nodes when resolving bootstrap call dependencies ([#&#8203;49010](angular/angular#49010)) |
| [26cb7ab2e6](angular/angular@26cb7ab) | perf | speed up language service lookups ([#&#8203;49010](angular/angular#49010)) |

##### platform-browser

| Commit | Type | Description |
| -- | -- | -- |
| [bf4ad38117](angular/angular@bf4ad38) | fix | remove styles from DOM of destroyed components ([#&#8203;48298](angular/angular#48298)) |

##### platform-server

| Commit | Type | Description |
| -- | -- | -- |
| [25e220a23a](angular/angular@25e220a) | fix | avoid duplicate TransferState info after renderApplication call ([#&#8203;49094](angular/angular#49094)) |

##### router

| Commit | Type | Description |
| -- | -- | -- |
| [31b94c762f](angular/angular@31b94c7) | feat | Add a withNavigationErrorHandler feature to provideRouter ([#&#8203;48551](angular/angular#48551)) |
| [dedac8d3f7](angular/angular@dedac8d) | feat | Add test helper for trigger navigations in tests ([#&#8203;48552](angular/angular#48552)) |

#### Special Thanks

Alan Agius, Alex Castle, Alex Rickabaugh, Andrew Kushnir, Andrew Scott, Dylan Hunn, Ikko Eltociear Ashimine, Ilyass, Jessica Janiuk, Joey Perrott, John Manners, Kalbarczyk, Kristiyan Kostadinov, Matthieu Riegler, Paul Gschwendtner, Pawel Kozlowski, Virginia Dooley, Walid Bouguima, cexbrayat and mgechev

<!-- CHANGELOG SPLIT MARKER -->

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about these updates again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNC4xNDkuMCIsInVwZGF0ZWRJblZlciI6IjM0LjE1Mi4wIn0=-->

Co-authored-by: cabr2-bot <cabr2.help@gmail.com>
Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1793
Reviewed-by: Epsilon_02 <epsilon_02@noreply.codeberg.org>
Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
@angular-automatic-lock-bot
Copy link

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Mar 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker aio: preview area: migrations Issues related to `ng update` migrations target: minor This PR is targeted for the next minor release
Projects
None yet
3 participants