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

migrating from nx 15.8.5 to 17.1.3 has missing migrations #20407

Closed
4 tasks
robertIsaac opened this issue Nov 24, 2023 · 16 comments
Closed
4 tasks

migrating from nx 15.8.5 to 17.1.3 has missing migrations #20407

robertIsaac opened this issue Nov 24, 2023 · 16 comments
Assignees
Labels
outdated scope: angular Issues related to Angular support in Nx type: bug

Comments

@robertIsaac
Copy link
Contributor

Current Behavior

these commits are missing

  1. update-17-0-0-remove-deprecated-build-options
  2. update-tsconfig-spec-jest
  3. migration-v16-guard-and-resolve-interfaces
  4. update-16-2-0-normalize-tsconfigs

Expected Behavior

these commits to exists

GitHub Repo

No response

Steps to Reproduce

  1. create a new workspace with angular and cypress included in version 15.8.5
  2. migrate to 17.1.3 (latest now)

Nx Report

Node   : 18.16.0
   OS     : win32-x64
   yarn   : 1.22.19

   nx (global)        : 17.1.3
   nx                 : 17.1.3
   @nx/js             : 17.1.3
   @nx/jest           : 17.1.3
   @nx/linter         : 17.1.3
   @nx/eslint         : 17.1.3
   @nx/workspace      : 17.1.3
   @nx/angular        : 17.1.3
   @nx/cypress        : 17.1.3
   @nx/devkit         : 17.1.3
   @nx/eslint-plugin  : 17.1.3
   @nx/plugin         : 17.1.3
   @nrwl/tao          : 17.1.3
   @nx/web            : 17.1.3
   @nx/webpack        : 17.1.3
   typescript         : 5.2.2
   ---------------------------------------
   Community plugins:
   @compodoc/compodoc       : 1.1.18
   @ngneat/transloco        : 3.1.4
   @ngrx/component          : 16.0.1
   @ngrx/component-store    : 16.0.1
   @ngrx/effects            : 16.0.1
   @ngrx/entity             : 16.0.1
   @ngrx/router-store       : 16.0.1
   @ngrx/schematics         : 16.0.1
   @ngrx/store              : 16.0.1
   @ngrx/store-devtools     : 16.0.1
   @testing-library/angular : 11.0.2
   ---------------------------------------
   Local workspace plugins:
         workspace-plugin

Failure Logs

NA

Package Manager Version

No response

Operating System

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

Additional Information

I want to explain what I was trying to do
our aim is to upgrade from angular 15 to 17, but we have some old unmerged branches that have big code changes regarding new features, and most probably we will need to run the migration for them (same happened when we upgraded from angular 13 to 15 regarding angular material legacy change, it made almost all of our unmerged branches broken)
when I'm upgrading it's very difficult to just move from one big version to another because too much changes that you can't handle and fix, so I moved one minor version at a time (to 15.9, 15.10, 16.0 ... till 17.1) and fixing all the errors (which were all fully compatible with angular 15), the problem now that migration.json has only the changes from nx 17.0 to nx 17.1
so I cherry picked these fix commits to a new branch then ran the nx migrate directly from 15.8.5 to latest so we have one migration.json that include everything so after we merge the upgrade the other branch can rebase on it and run the nx migrate --run-migrations to apply these fixes in the old branches as well
after I finished I compared the two branches and found there are +100 difference
so I started comparing commits (since I'm using nx migrate --run-migrations --create-commits and found these commits missing

  1. update-17-0-0-remove-deprecated-build-options (doesn't exist in the migration.json)
  2. update-tsconfig-spec-jest (exists in the migration.json, but was skipped for unknown reason)
  3. migration-v16-guard-and-resolve-interfaces (doesn't exist in the migration.json)
  4. update-16-2-0-normalize-tsconfigs (exists in the migration.json, but was skipped for unknown reason)
@AgentEnder AgentEnder added the scope: angular Issues related to Angular support in Nx label Dec 1, 2023
@leosvelperez
Copy link
Member

Thanks for reporting this!

That's an interesting flow to update your repo, and while it should work for the most part, it's not guaranteed to work.

The first thing is that the migrations contained in the @angular/core package are only kept for one major version. The Angular CLI doesn't support updating multiple major versions at a time and therefore, their packages only contain migrations for any given current major version.

So, going from Nx 15.8.5 to Nx 17.1.3 means going from Angular 15.2.x to Angular 17.0.x. That's jumping two Angular major versions. The installed Angular 17.0.x package will only contain migrations for Angular v17 and the migration-v16-guard-and-resolve-interfaces migration is from Angular v16. That's why that migration doesn't exist in the migrations.json file.

The solution is to update one major version at a time. This is what we recommend in our docs, though we should probably make it more visible and potentially call out Angular explicitly.

Regarding the other three migrations, when I followed your reproduction steps, I did get all of them in the migrations.json file (including update-17-0-0-remove-deprecated-build-options), but there was no commit for any of them. Now, that's not necessarily an issue. Not all migrations result in code changes. They are meant to update code only if needed. When a migration doesn't make a change, a commit is not created.

Now, in the fresh 15.8.5 workspace I used to reproduce this, I noticed the following:

  • update-17-0-0-remove-deprecated-build-options: there were no relevant targets to update. It didn't make any changes correctly.
  • update-tsconfig-spec-jest: should have made changes but it didn't because the migration checks for targets with the @nrwl/jest:jest executor, but that executor was renamed in a migration that targets a higher version of Nx but was run before this one
  • update-16-2-0-normalize-tsconfigs: there was no tsconfig.cy.json files to update. It didn't make any changes correctly.

I can push a fix for the update-tsconfig-spec-jest to consider the renamed executor. A more involved (but more correct fix) would be to update nx migrate to run the migrations sorted by their target version. I'll bring the latter internally.

If in your workspace you think the other two migrations should have generated changes and didn't do so incorrectly, please provide a reproduction so we can troubleshoot them.

@leosvelperez leosvelperez self-assigned this Feb 12, 2024
@robertIsaac
Copy link
Contributor Author

robertIsaac commented Feb 12, 2024

@leosvelperez
I'm sure the other two produced commits, because I was comparing commits not the migration files
I will try tomorrow to get back the deleted branches and give you what was the change, but since it has been a long time I'm not sure if I would be able to find it

@leosvelperez
Copy link
Member

Yeah, that's why I'd need a reproduction to see what the issue could be. Following your reproduction steps left me with what I mentioned above, where there's only one migration with an issue. I'm not saying there are no issues with the others. It's just I haven't been able to reproduce them.

I know it has been a while and I apologize for the delay on getting to this. It sounds you were able to move forward, so I'll push the fix for the one I could reproduce the issue and know what happened. I appreciate the time you've put to provide the information here and if you can't find the issues, that's ok. Let us know if that's the case so we can close this. We'll keep an eye on this anyways.

@leosvelperez
Copy link
Member

@robertIsaac I fixed the issue with running migrations out of order in #21799. That fixes the issue you faced with the update-tsconfig-spec-jest migration.

I'm going to close this issue given you mentioned it might be difficult to retrieve the relevant information for the other issues you found. If you later find any relevant information, please create a new issue with the details and link back to this issue. You can tag me and I'll take a look.

@robertIsaac
Copy link
Contributor Author

Thanks @leosvelperez for fixing that one
I couldn't find the old branches but fixing that one might fix the others as well

@robertIsaac
Copy link
Contributor Author

hi @leosvelperez
I found the commits
should I open a new ticket or this comment is enough

chore: [nx migration] update-17-0-0-remove-deprecated-build-options

image

chore: [nx migration] update-16-2-0-normalize-tsconfigs

image

@robertIsaac
Copy link
Contributor Author

FYI the option that was removed in chore: [nx migration] update-17-0-0-remove-deprecated-build-options was actually added in a previous commit called chore: [nx migration] explicitly-set-projects-to-update-buildable-deps see it's details
image

@leosvelperez
Copy link
Member

@robertIsaac thanks for the new info!

I'm reopening this and I'll try to set some time later this week to check this. I think I know how to reproduce update-17-0-0-remove-deprecated-build-options based on the screenshots, but I might need more details for update-16-2-0-normalize-tsconfigs. It would help if you could provide all the tsconfig files (at the root and nested if you had any) inside the cypress-tests package before running that migration.

@leosvelperez leosvelperez reopened this Feb 19, 2024
@robertIsaac
Copy link
Contributor Author

robertIsaac commented Feb 21, 2024

{
  "compilerOptions": {
    "allowJs": true,
    "outDir": "../../dist/out-tsc",
    "sourceMap": false,
    "strict": true,
    "types": [
      "cypress",
      "node",
      "cypress-real-events",
      "@testing-library/cypress",
      "cypress-wait-until",
      "cypress-xpath"
    ]
  },
  "extends": "../../tsconfig.base.json",
  "include": ["cypress/**/*.ts", "cypress/**/*.js", "./**/cypress.config.ts"]
}

that was the only one

the is

{
  "angularCompilerOptions": {
    "strictInjectionParameters": true,
    "strictInputAccessModifiers": true,
    "strictTemplates": true
  },
  "compileOnSave": false,
  "compilerOptions": {
    "allowSyntheticDefaultImports": true,
    "allowUnreachableCode": false,
    "allowUnusedLabels": false,
    "baseUrl": ".",
    "declaration": false,
    "downlevelIteration": true,
    "emitDecoratorMetadata": true,
    "esModuleInterop": true,
    "exactOptionalPropertyTypes": true,
    "experimentalDecorators": true,
    "forceConsistentCasingInFileNames": true,
    "importHelpers": true,
    "lib": ["es2018", "dom"],
    "module": "esnext",
    "moduleResolution": "node",
    "noFallthroughCasesInSwitch": true,
    "noImplicitOverride": true,
    "noImplicitReturns": true,
    "noPropertyAccessFromIndexSignature": true,
    "noUncheckedIndexedAccess": true,
    "noUnusedLocals": true,
    "noUnusedParameters": true,
    "paths": {
      "our-internal-libraries": ["...."]
     },
    "resolveJsonModule": true,
    "rootDir": ".",
    "skipLibCheck": true,
    "sourceMap": true,
    "strict": true,
    "target": "es2015"
  },
  "exclude": ["node_modules", "tmp"]
}

@leosvelperez
Copy link
Member

Can you confirm the name of the tsconfig file before the migration? Was it tsconfig.json or something else?

@robertIsaac
Copy link
Contributor Author

robertIsaac commented Feb 21, 2024

yes tsconfig.json
I noticed something now
the change introduced by chore: [nx migration] update-16-2-0-normalize-tsconfigs was actually reverted by one of my colleagues, not sure why though, will ask him when he is online (he is in different timezone)

edit: because we don't have tsconfig.json file inside cypress folder, now I'm not sure why this was added at all

@leosvelperez
Copy link
Member

Good to know. That rules out that it wasn't applied. There might be a different issue leading your colleague to remove it, but it would be something separate.

edit: because we don't have tsconfig.json file inside cypress folder, now I'm not sure why this was added at all

If there was no tsconfig.json inside the cypress folder, the migration would make some changes.

@leosvelperez
Copy link
Member

@robertIsaac I confirm the issue with update-17-0-0-remove-deprecated-build-options is also due to migrations not running ordered by the version they target. So, the issue was addressed by the fix I pushed last week.

I'm closing this one since the issues have been addressed and the fix will be released in 18.1.0.

@robertIsaac
Copy link
Contributor Author

Thanks for fixing it, I think future migration will be much better now
One question I have
Do you recommend specific way to migrate between major version?
We always have a compatibility issue that delay our migration, by the time we fix it, it's already 2 major version (now for example our issue is with Angular material we are still using the legacy one which is not supported in version 17, and version 18 of Angular won't work with material 16)

@leosvelperez
Copy link
Member

The main piece of advice is to limit the version jump to, at most, one major at a time. In the case of Angular, this is actually required due to the Angular packages only keeping migrations for a single major version. Regardless, even though we all try our best to provide a solid migration experience, migrating code it's very difficult. There are so many variations/deviations workspaces can have out there that's very difficult, if not impossible, to account for every setup and migrate it successfully. We also make mistakes, and it adds up. This risk increases the more versions a migration jumps.

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 Mar 25, 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

No branches or pull requests

3 participants