feat(core): respect packageGroup in nx migrate #9667

gioragutt · 2022-04-02T18:26:38Z

Current Behavior

package.json#ng-update.packageGroup is not respected by nx migrate.

Expected Behavior

The following configurations in package.json should work:

"ng-update": {
  "migrations": "./migrations.json",
  "packageGroup": [
    "@my-company/my-package-1",
    "@my-company/my-package-2",
    {
      "package": "@my-company/my-package-3",
      "version": "latest", // any version/tag respected by package managers would work
    },
    {
      "package": "@my-company/my-package-3",
      "version": "1.0.0",
    }
  ]
},

// nx-migrations is the equivalent of ng-update for nx
"nx-migrations": {
  "migrations": "./migrations.json",
  "packageGroup": [
    "@my-company/my-package-1",
    "@my-company/my-package-2",
    {
      "package": "@my-company/my-package-3",
      "version": "latest", // any version/tag respected by package managers would work
    },
    {
      "package": "@my-company/my-package-3",
      "version": "1.0.0",
    }
  ]
}

Related Issue(s)

Fixes #4575

ISSUES CLOSED: nrwl#4575

nx-cloud · 2022-04-02T18:26:40Z

☁️ Nx Cloud Report

CI is running/has finished running commands for commit d2969e8. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

✅ Successfully ran 7 targets

Sent with 💌 from NxCloud.

vercel · 2022-04-02T18:26:43Z

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/nrwl/nx-dev/3t9n2TAFp1Zcqu1bW6yWGN5RRiTB
✅ Preview: Canceled

[Deployment for d2969e8 canceled]

gioragutt · 2022-04-02T18:45:04Z

packages/workspace/package.json

+      "@nrwl/web",
+      "@nrwl/js",
+      "@nrwl/cli",
+      "@nrwl/nx-cloud",


Should the ng-update section remain here if nx-migrations works the same?

It shouldn't be necessary for nx plugins, but there may be some cases in really old repos that have @nrwl/workspace installed as a plugin for angular CLI. @FrozenPandaz can you review this piece? I'm not sure if its still needed or not.

gioragutt · 2022-04-02T20:06:48Z

@AgentEnder @FrozenPandaz I know the PR is a lot, so please let me know if I can assist in any way, questions or whatever needed

AgentEnder

Hey @gioragutt! Thanks for the PR, its a large amount of work and contains some helpful clean ups too!

This is functionality that people have complained about missing for a while, and is something that should certainly be added imho. I'll work on getting a few additional reviews on this PR, since its large and touches some areas that recently changed.

@FrozenPandaz: Do you care to give this a sanity check? Other than my notes it LGTM, but you may have some additional concerns + context.
@leosvelperez: You recently changed a bit of things in migrate.ts to make it faster. Do you care to glance over the portions you touched that are also touched here and sanity check them?
Thanks again!

packages/nx/src/command-line/migrate.ts

AgentEnder · 2022-04-04T18:29:22Z

packages/workspace/package.json

+      "@nrwl/web",
+      "@nrwl/js",
+      "@nrwl/cli",
+      "@nrwl/nx-cloud",


It shouldn't be necessary for nx plugins, but there may be some cases in really old repos that have @nrwl/workspace installed as a plugin for angular CLI. @FrozenPandaz can you review this piece? I'm not sure if its still needed or not.

AgentEnder · 2022-04-04T18:31:49Z

packages/nx/src/command-line/migrate.ts

+          return {
+            ...acc,
+            [pkg]: {
+              version,
              alwaysAddToPackageJson: false,


Should we allow this part to be specc'd from the packageGroup object? Imagine a plugin introduces a new utils package, should they have to write a migration that installs it or should they be able to add it to packageGroup with alwaysAddToPackageJson set to true?

If I allow version (to support nx-cloud), I can definitely allow the rest of the keys if wanted.

I'm not sure that all of them would be needed, and I actually think it may be fine to leave it as is. @FrozenPandaz opinion?

packages/nx/src/utils/package-manager.ts

AgentEnder · 2022-04-04T20:18:01Z

@gioragutt I think that CI should run properly on this PR now that I pushed up that empty commit, I'm not sure why Circle acts like this sometimes, you've got merged contributions so I'd expect it to work without me pushing 🤷

gioragutt · 2022-04-05T06:59:32Z

@AgentEnder there are failing tests in packages/nx (hurray for tests 🎉), I'll handle the fixes

gioragutt · 2022-04-05T08:01:53Z

@AgentEnder had to revert the normalizeVersion changes since it does some really specific parsing and UTs fail, I tried different variations of using the semver methods but the tests check for very specific behavior.

Perhaps it is worth re-iterating over it later, since it might create a more standard behavior, but currently fixing it would introduce a breaking change of normalizeVersion.

And, as always, your commit is needed to trigger CI 😅

AgentEnder

LGTM now 🎉 . Going to talk over changes with @vsavkin and @FrozenPandaz sometime this week before figuring out appropriate time to merge.

AgentEnder · 2022-04-07T13:08:37Z

packages/nx/src/command-line/migrate.ts

+        packages: migration.packageGroup.reduce((acc, packageConfig) => {
+          const { package: pkg, version } =
+            typeof packageConfig === 'string'
+              ? { package: packageConfig, version: targetVersion }
+              : packageConfig;
+
+          return {
+            ...acc,
+            [pkg]: {
+              version,


Talked to vsavkin a bit more, and there should probably be a recursive portion of this.

Collapse packages would need to support packages in the packageGroup, which also have a packageGroup themselves. There should also be some circular prevention. I.e.:

a: ['b', 'c',] b: ['a', 'd'] c: [] d: []

nx migrate a should expand into a, b, c, then b should be expanded into b, a, d, then c and d shouldn't expand since they have no group. The function would need to notice that its already expanded a, and refrain from doing so when it handles expanding b.

Does that make sense?

Yeah, it does make sense, I'll make it recursive, and will try to make it a little more readable in the process, the code is pretty hard to read currently.

@AgentEnder @vsavkin I added a UT before starting to work on this, and I was surprised to see the test pass 😂

I looked at the code and than I saw that _updatePackageJson already recursively calls itself, so this is already handled 👍🏻

I'll upload the test just so this will be covered.

AgentEnder

This looks good, just found a small change during review with Jason. We are going to go ahead and merge, and put in the change real quick afterwards.

Thanks for the contribution @gioragutt

AgentEnder · 2022-04-08T18:53:02Z

packages/nx/src/utils/package-json.ts

@@ -9,9 +9,15 @@ export interface NxProjectPackageJsonConfiguration {
  targets?: Record<string, PackageJsonTargetConfiguration>;
 }

+export interface NxMigrationsConfiguration {
+  migrations?: string;
+  packageGroup?: (string | { package: string; version: string })[];


Package group can be an object, defined as Record<packageName, version>. I like the addition of being able to expand a single package and declare its version, so lets keep that but also add support for the format w/ version that angular specifies. For reference, check the package group in @angular/cli compared to @ngrx/store:

https://unpkg.com/@ngrx/store@13.1.0/package.json
https://unpkg.com/@angular/cli@13.3.2/package.json

Oh yeah, that's a good idea, I wasn't aware that this was one of the formats. It should be easy enough to add support for this, as ResolvedMigrationConfiguration is separated from NxMigrationConfiguration

Yeah, we had some follow up work in migrate.ts anyways 😄

FrozenPandaz · 2022-04-08T19:57:19Z

Awesome work @gioragutt! Thank you so much, we were actually planning on tackling this soon. LGTM 🎉

github-actions · 2023-03-18T21:04:57Z

This pull request has already been merged/closed. If you experience issues related to these changes, please open a new issue referencing this pull request.

gioragutt added 2 commits April 2, 2022 17:49

cleanup(core): refactor migrate.ts

904a3e5

feat(core): respect packageGroup in nx migrate

d6ccd70

ISSUES CLOSED: nrwl#4575

vercel bot temporarily deployed to Preview April 2, 2022 18:26 Inactive

gioragutt commented Apr 2, 2022

View reviewed changes

AgentEnder requested review from AgentEnder and FrozenPandaz April 2, 2022 18:51

fix build

950026c

vercel bot temporarily deployed to Preview April 2, 2022 20:50 Inactive

AgentEnder requested changes Apr 4, 2022

View reviewed changes

AgentEnder requested a review from leosvelperez April 4, 2022 18:48

chore(repo): trigger CI

85d3ec6

vercel bot temporarily deployed to Preview April 4, 2022 19:56 Inactive

chore(core): fix review comments, without packageGroup extension

6dc4bdc

vercel bot temporarily deployed to Preview April 4, 2022 20:32 Inactive

chore(core): revert withTempNpmDirectory changes

5493ed9

vercel bot temporarily deployed to Preview April 4, 2022 20:41 Inactive

gioragutt requested a review from AgentEnder April 4, 2022 20:42

chore(repo): trigger ci

e3e51d4

vercel bot temporarily deployed to Preview April 4, 2022 21:50 Inactive

chore(core): fix unit tests

7e0f331

vercel bot temporarily deployed to Preview April 5, 2022 07:57 Inactive

chore(core): remove view and pack from pmc

8a8db11

vercel bot temporarily deployed to Preview April 5, 2022 13:08 Inactive

chore(repo): kick off CI

97470ca

vercel bot temporarily deployed to Preview April 5, 2022 13:36 Inactive

AgentEnder approved these changes Apr 5, 2022

View reviewed changes

AgentEnder requested changes Apr 7, 2022

View reviewed changes

AgentEnder removed the request for review from leosvelperez April 7, 2022 15:15

chore(core): add tests for recursive packageGroup

06bf0fb

vercel bot temporarily deployed to Preview April 7, 2022 15:18 Inactive

gioragutt requested a review from AgentEnder April 7, 2022 15:19

vercel bot temporarily deployed to Preview April 7, 2022 16:37 Inactive

chore(core): add another test for cyclic packageGroup

6b4dfda

gioragutt force-pushed the giorag/nx-migrate-package-groups branch from 0a3f70f to 6b4dfda Compare April 7, 2022 16:41

vercel bot temporarily deployed to Preview April 7, 2022 16:41 Inactive

chore(repo): kickoff CI

d2969e8

vercel bot temporarily deployed to Preview April 7, 2022 17:43 Inactive

AgentEnder approved these changes Apr 8, 2022

View reviewed changes

FrozenPandaz merged commit 32b49b6 into nrwl:master Apr 8, 2022

gioragutt deleted the giorag/nx-migrate-package-groups branch April 8, 2022 20:00

gioragutt mentioned this pull request Apr 8, 2022

feat(nx-plugin): add plugin-lint executor #9697

Merged

github-actions bot locked as resolved and limited conversation to collaborators Mar 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(core): respect packageGroup in nx migrate #9667

feat(core): respect packageGroup in nx migrate #9667

gioragutt commented Apr 2, 2022

nx-cloud bot commented Apr 2, 2022 •

edited

Loading

vercel bot commented Apr 2, 2022 •

edited

Loading

gioragutt Apr 2, 2022

AgentEnder Apr 4, 2022

gioragutt commented Apr 2, 2022

AgentEnder left a comment

AgentEnder Apr 4, 2022

AgentEnder Apr 4, 2022

gioragutt Apr 4, 2022

AgentEnder Apr 4, 2022

AgentEnder commented Apr 4, 2022

gioragutt commented Apr 5, 2022

gioragutt commented Apr 5, 2022

AgentEnder left a comment

AgentEnder Apr 7, 2022

gioragutt Apr 7, 2022

gioragutt Apr 7, 2022

AgentEnder left a comment

AgentEnder Apr 8, 2022

gioragutt Apr 8, 2022

AgentEnder Apr 8, 2022

FrozenPandaz commented Apr 8, 2022

github-actions bot commented Mar 18, 2023

feat(core): respect packageGroup in nx migrate #9667

feat(core): respect packageGroup in nx migrate #9667

Conversation

gioragutt commented Apr 2, 2022

Current Behavior

Expected Behavior

Related Issue(s)

nx-cloud bot commented Apr 2, 2022 • edited Loading

☁️ Nx Cloud Report

vercel bot commented Apr 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gioragutt commented Apr 2, 2022

AgentEnder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AgentEnder commented Apr 4, 2022

gioragutt commented Apr 5, 2022

gioragutt commented Apr 5, 2022

AgentEnder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AgentEnder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FrozenPandaz commented Apr 8, 2022

github-actions bot commented Mar 18, 2023

nx-cloud bot commented Apr 2, 2022 •

edited

Loading

vercel bot commented Apr 2, 2022 •

edited

Loading