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

Ngrx18 migration incomplete - migration for breaking changes are not included #26694

Closed
1 of 4 tasks
colinscz opened this issue Jun 26, 2024 · 7 comments · Fixed by #27107
Closed
1 of 4 tasks

Ngrx18 migration incomplete - migration for breaking changes are not included #26694

colinscz opened this issue Jun 26, 2024 · 7 comments · Fixed by #27107
Assignees
Labels
outdated scope: angular Issues related to Angular support in Nx type: bug

Comments

@colinscz
Copy link

colinscz commented Jun 26, 2024

Current Behavior

Currently some of the ngrx dependencies are automatically updated. As introduced in this PR: https://github.com/nrwl/nx/pull/26549/files

But there are some breaking changes and new packages introduced in certain scenarios that are not yet covered by the migration. This results in the build failing because for example this import right here:
import { concatLatestFrom } from '@ngrx/effects';

is not found anymore and newly reside in the other package:
import { concatLatestFrom } from '@ngrx/operators';

More info here: https://ngrx.io/guide/migration/v18

Could you please add migrations for those scenarios?

I checked the open PRs and issues but I haven't found any planned change for this.

Expected Behavior

Breaking changes from Ngrx migration including changes for some code moved from @ngrx/effects to @ngrx/operators are included in the migrations.

GitHub Repo

No response

Steps to Reproduce

  1. Update from a Nx Angular workspace containing more than just the ngrx store dependency (in my case I was on Nx 17.3.2)
  2. Try to run the application after this upgrade and after all necessary migrations have been applied

Nx Report

Nx report from before the nx migrate latest command was executed since after that I already have issues with the npm install:

   Node   : 18.20.2
   OS     : win32-x64
   npm    : 10.5.0
   
   nx (global)        : 19.3.1
   nx                 : 17.3.2
   @nx/js             : 17.3.2
   @nx/jest           : 17.3.2
   @nx/linter         : 17.3.2
   @nx/eslint         : 17.3.2
   @nx/workspace      : 17.3.2
   @nx/angular        : 17.3.2
   @nx/cypress        : 17.3.2
   @nx/devkit         : 17.3.2
   @nx/eslint-plugin  : 17.3.2
   @nrwl/tao          : 17.3.2
   @nx/web            : 17.3.2
   @nx/webpack        : 17.3.2
   typescript         : 5.3.3
   ---------------------------------------
   Community plugins:
   @ngrx/component      : 17.0.1
   @ngrx/data           : 17.0.1
   @ngrx/effects        : 17.0.1
   @ngrx/entity         : 17.0.1
   @ngrx/router-store   : 17.0.1
   @ngrx/store          : 17.0.1
   @ngrx/store-devtools : 17.0.1
   eslint-plugin-ngrx   : 2.1.4
   nx-stylelint         : 17.1.4

Failure Logs

After the migrations to Nx 19.3.1 were run in the project

$ npm install
npm ERR! code ERESOLVE
npm ERR! ERESOLVE unable to resolve dependency tree
npm ERR!
npm ERR! While resolving: [email protected]
npm ERR! Found: @angular/[email protected]
npm ERR! node_modules/@angular/common
npm ERR!   @angular/common@"18.0.4" from the root project
npm ERR!
npm ERR! Could not resolve dependency:
npm ERR! peer @angular/common@"^17.0.0" from @ngrx/[email protected]
npm ERR! node_modules/@ngrx/component
npm ERR!   @ngrx/component@"17.0.1" from the root project
npm ERR!
npm ERR! Fix the upstream dependency conflict, or retry
npm ERR! this command with --force or --legacy-peer-deps
npm ERR! to accept an incorrect (and potentially broken) dependency resolution.
npm ERR!
npm ERR!

// even if I fix this one manually there are more dependencies not up to date that were as far as I can remember previously 
// included in updates:

(imported as 'concatLatestFrom') was not found in '@ngrx/effects' ([39m[22m[1m[31mpossible exports: Actions, EFFECTS_ERROR_HANDLER, EffectSources, EffectsFeatureModule, EffectsModule, EffectsRootModule, EffectsRunner, ROOT_EFFECTS_INIT, USER_PROVIDED_EFFECTS, act, createEffect, defaultEffectsErrorHandler, getEffectsMetadata, mergeEffects, ofType, provideEffects, rootEffectsInit)

[91merror[0m[90m TS2305: [0mModule '"@ngrx/effects"' has no exported member 'concatLatestFrom'.[39m[22m
[1m[31m[39m[22m
[1m[31m[7m5[0m import {Actions, concatLatestFrom, createEffect, ofType} from '@ngrx/effects';[39m[22m

Package Manager Version

No response

Operating System

  • macOS
  • Linux
  • Windows
  • Other (Please specify)

Additional Information

Unfortunately I cannot provide an exact repo and nx-examples does not use ngrx-store and related packages to that extend.

But happy to help in case I can assist with any additional information.

@AgentEnder AgentEnder added the scope: angular Issues related to Angular support in Nx label Jul 8, 2024
@leosvelperez
Copy link
Member

Thanks for reporting this!

Unfortunately, I can't reproduce the issue. Based on the provided information I tried:

  • created a fresh Nx 17.3.2 (the version you referred you were on) workspace
  • created an app and a lib
  • set up NgRx in both projects with the effect files importing concatLatestFrom from @ngrx/effects
  • ran nx migrate latest
  • ran nx migrate --run-migrations

After that, the effect files were correctly updated. The concatLatestFrom operator was changed to be imported from @ngrx/operators, and that package was installed in the workspace.

Please provide a reproduction so we can troubleshoot the issue.

@colinscz
Copy link
Author

@leosvelperez thank you for trying this out. I am looking to do the same as there seems to be an issue with an older migration not being correctly applied. So before going to 17.3.2 I was on 16.4.0. I am now trying locally to go from 17.3.2 first to 18.3.5 before going to the latest version.
If it's possible for you to share your tryout project that would be great.

I saw there were some old migrations added and ported back to include the @ngrx/operators package here: #21417
I am using ngrx but the operator package was never added in the past migrations. I usually just bump to the latest new released major version.

I don't know if it should have been automatically added by the ngrx 17 to ngrx 18 migration if it wasn't there before. At least that was my expectation. 🤔

So what I did for now I installed the @ngrx/operators package manually, bumping Nx to 18.3.5 and afterwards will try to go through with the update to 19.5.1.

I post again once I am done with that experiment.

@colinscz
Copy link
Author

colinscz commented Jul 22, 2024

@leosvelperez just tried it out. Looks like the migration command (nx migrate latest) from 18.3.5 to 19.5.1 does not bump the @ngrx/operators package (if it is already listed as a dependency) to the same version but it does it correctly for all other @ngrx packages. 🤔
grafik

what I did next
I bumped it manually to match the other @ngrx packages, executed the npm install and ran the migrations according to the Nx instructions:

 Next steps:

- Make sure package.json changes make sense and then run 'npm install',
- Run 'npx nx migrate --run-migrations'
- To learn more go to https://nx.dev/features/automate-updating-dependencies
- You may run 'npm run nx -- connect-to-nx-cloud' to get faster builds, GitHub integration, and more. Check out https://nx.app

But it looks like the issue is the same. The migration fails once it gets to the migration step for replacing the imports for @ngrx/effects to @ngrx/operators according to the output.

    [@ngrx/effects] Updated concatLatestFrom to import from '@ngrx/operators'
    [@ngrx/effects] Updated concatLatestFrom to import from '@ngrx/operators'
    [@ngrx/effects] Updated concatLatestFrom to import from '@ngrx/operators'

 NX   Failed to run ngrx-effects-migration-18-beta from @ngrx/effects. This workspace is NOT up to date!


 NX   Index 690 outside of range [0, 486].

I'll try to create a repro with the nx-examples repository in case your tryout project is not directly available. :)

More detailed logs, took some time because I had to replace the confidential stuff.

    [@ngrx/effects] Updated concatLatestFrom to import from '@ngrx/operators'
    [@ngrx/effects] Updated concatLatestFrom to import from '@ngrx/operators'
    [@ngrx/effects] Updated concatLatestFrom to import from '@ngrx/operators'

 NX   Failed to run ngrx-effects-migration-18-beta from @ngrx/effects. This workspace is NOT up to date!


 NX   Index 690 outside of range [0, 486].


Error: Index 690 outside of range [0, 486].
    at UpdateRecorderBase._assertIndex (C:\Develop\parent-project\child-project\nx-workspace\node_modules\@angular-devkit\schematics\src\tree\recorder.js:65:19)
    at UpdateRecorderBase.remove (C:\Develop\parent-project\child-project\nx-workspace\node_modules\@angular-devkit\schematics\src\tree\recorder.js:80:14)
    at createChangeRecorder (C:\Develop\parent-project\child-project\nx-workspace\node_modules\@ngrx\effects\schematics-core\utility\change.js:135:26)
    at commitChanges (C:\Develop\parent-project\child-project\nx-workspace\node_modules\@ngrx\effects\schematics-core\utility\change.js:157:20)
    at C:\Develop\parent-project\child-project\nx-workspace\node_modules\@ngrx\effects\migrations\18_0_0-beta\index.js:89:49
    at visitTSSourceFiles (C:\Develop\parent-project\child-project\nx-workspace\node_modules\@ngrx\effects\schematics-core\utility\visitors.js:66:22)
    at C:\Develop\parent-project\child-project\nx-workspace\node_modules\@ngrx\effects\migrations\18_0_0-beta\index.js:37:50
    at callRuleAsync (C:\Develop\parent-project\child-project\nx-workspace\node_modules\@angular-devkit\schematics\src\rules\call.js:77:24)
    at C:\Develop\parent-project\child-project\nx-workspace\node_modules\@angular-devkit\schematics\src\rules\call.js:73:40
    at Observable._subscribe (C:\Develop\parent-project\child-project\nx-workspace\node_modules\rxjs\dist\cjs\internal\observable\defer.js:8:31)
Command failed: npx nx _migrate --run-migrations --verbose

 NX   Command failed: npx nx migrate --run-migrations --verbose


Error: Command failed: npx nx migrate --run-migrations --verbose
    at checkExecSyncError (node:child_process:890:11)
    at execSync (node:child_process:962:15)
    at runNxSync (C:\Develop\APPDAT~1\21\tmp-39832-xycx77Sd445b\node_modules\nx\src\utils\child-process.js:27:34)
    at runMigrations (C:\Develop\APPDAT~1\21\tmp-39832-xycx77Sd445b\node_modules\nx\src\command-line\migrate\migrate.js:964:39)
    at C:\Develop\APPDAT~1\21\tmp-39832-xycx77Sd445b\node_modules\nx\src\command-line\migrate\migrate.js:1033:19
    at async handleErrors (C:\Develop\APPDAT~1\21\tmp-39832-xycx77Sd445b\node_modules\nx\src\utils\params.js:22:24)
Command failed: C:\develop\APPDAT~1\21\tmp-39832-xycx77Sd445b\node_modules\.bin\nx _migrate --run-migrations --verbose

@colinscz
Copy link
Author

@leosvelperez seems like I found the issue: ngrx/platform#4397
Since Nx is pointing to the version with the migration bug it causes issues during the upgrade. So I assume bumping the version to 18.0.1 fixes this.

I tried it out and damn it works. After bumping all @ngrx packages to v18.0.1 I have no issues running the migrations. 🥳
grafik

This probably means one or more bugfix releases for Nx so the broken Ngrx setup in v18.0.0 is not referenced anymore. Don't shoot the messenger 😄 😉

@leosvelperez
Copy link
Member

@colinscz thanks for the detective work here! I'll add a migration to bump the version to 18.0.1 or later.

For some reason, the NgRx team doesn't include the @ngrx/operators in the @ngrx/store package group. We update all NgRx packages through that package group, which is supposed to list all the packages that update together. Given the package is not listed there, I'll add it to our migrations to ensure it's updated.

@colinscz
Copy link
Author

@leosvelperez welcome man, thanks for the quick fix and happy to help :)

That is indeed weird. Not sure what the reason is. 🤷🏽‍♂️

FrozenPandaz pushed a commit that referenced this issue Jul 30, 2024
…e update list (#27107)

<!-- Please make sure you have read the submission guidelines before
posting an PR -->
<!--
https://github.com/nrwl/nx/blob/master/CONTRIBUTING.md#-submitting-a-pr
-->

<!-- Please make sure that your commit message follows our format -->
<!-- Example: `fix(nx): must begin with lowercase` -->

<!-- If this is a particularly complex change or feature addition, you
can request a dedicated Nx release for this pull request branch. Mention
someone from the Nx team or the `@nrwl/nx-pipelines-reviewers` and they
will confirm if the PR warrants its own release for testing purposes,
and generate it for you if appropriate. -->

## Current Behavior
<!-- This is the behavior we have today -->

## Expected Behavior
<!-- This is the behavior we should expect with the changes in this PR
-->

## Related Issue(s)
<!-- Please link the issue being fixed so it gets closed when this is
merged. -->

Fixes #26694

(cherry picked from commit ebb42b7)
Copy link

This issue has been closed for more than 30 days. If this issue is still occuring, please open a new issue with more recent context.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 27, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated scope: angular Issues related to Angular support in Nx type: bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants