refactor(@angular/cli): improve update package discovery #18610

andreialecu · 2020-08-25T14:15:52Z

Removes the dependency on read-package-tree which is closely tied to npm.

andreialecu · 2020-08-27T14:21:04Z

If require.resolve is used instead of resolve from npm, the following error occurs in e2e tests (as of August 2020):

An unhandled exception occurred: Cannot find module 'rxjs-compat'
Require stack:
- /tmp/angular-cli-e2e-CLpXdU/assets/1.0-project-1598382026407/node_modules/rxjs/Rx.js
- /tmp/angular-cli-e2e-CLpXdU/assets/1.0-project-1598382026407/node_modules/@angular/core/schematics/migrations/static-queries/index.js
- /tmp/angular-cli-e2e-CLpXdU/assets/1.0-project-1598382026407/node_modules/@angular-devkit/schematics/tools/export-ref.js
- /tmp/angular-cli-e2e-CLpXdU/assets/1.0-project-1598382026407/node_modules/@angular-devkit/schematics/tools/index.js

The problem is with the use of require.resolve to find the location of dependencies, populating this NodeJS internal cache:
https://github.com/nodejs/node/blob/dcba12895ad58275ba5b027c2c5110461dfebf66/lib/internal/modules/cjs/loader.js#L239

rxjs@5.5.12 had the main entry in package.json set to ./Rx.js (https://unpkg.com/rxjs@5.5.12/package.json)

rxjs@6.6.2 has it set to ./index.js (https://unpkg.com/rxjs@6.6.2/package.json)

Here's a step by step explanation:

The code gathers package.json dependencies, then visits each dependency's package.json to check which version is actually installed (NodeJS packageJsonCache gets populated, see link above). At this point rxjs is at version 5.5.12
yarn/npm install is ran and rxjs is updated to version 6.6.2.
executeMigrations tries to run the schematics in the context of the same process that ran step 1. rxjs will resolve to rxjs/Rx.js instead of rxjs/index.js, and thus the process fails

The problem is also described here: https://stackoverflow.com/questions/59865584/how-to-invalidate-cached-require-resolve-results

A simpler repro is here: https://gist.github.com/andreialecu/748bb0e48396a3f217b3de912489af07

Replacing require.resolve with https://www.npmjs.com/package/resolve fixes this.

andreialecu · 2020-08-27T17:49:49Z

I also included an E2E test for a yarn workspaces monorepo and it seems to pass.

andreialecu · 2020-08-28T09:22:48Z

There are still some potential problems and edge cases, which cannot be fixed just in this package.

Particularly in this migration script:
https://github.com/angular/angular/blob/master/packages/core/schematics/migrations/missing-injectable/transform.ts#L9-L12

The script above depends on @angular/compiler-cli and typescript and neither of them are dependencies of @angular/core: https://unpkg.com/@angular/core@10.0.0/package.json

This causes problems if yarn installs the packages in different directories. For example I have a project where @angular/cli and most deps are hoisted to the top level of the monorepo, but @angular/compiler-cli is nested inside one particular workspace. If the dependency was defined properly, yarn would have a better hint of where to place dependencies within the file system and this error wouldn't exist.

Here's the error it can produce:

An unhandled exception occurred: Cannot find module '@angular/compiler-cli/src/ngtsc/imports'
Require stack:
- /home/aandrei/projects/monorepo/node_modules/@angular/core/schematics/migrations/missing-injectable/transform.js
- /home/aandrei/projects/monorepo/node_modules/@angular/core/schematics/migrations/missing-injectable/index.js
- /home/aandrei/projects/monorepo/node_modules/@angular-devkit/schematics/tools/export-ref.js
- /home/aandrei/projects/monorepo/node_modules/@angular-devkit/schematics/tools/index.js
- /home/aandrei/projects/monorepo/node_modules/@angular/cli/utilities/json-schema.js
- /home/aandrei/projects/monorepo/node_modules/@angular/cli/models/command-runner.js
- /home/aandrei/projects/monorepo/node_modules/@angular/cli/lib/cli/index.js
- /home/aandrei/projects/monorepo/node_modules/@angular/cli/lib/init.js
- /home/aandrei/projects/monorepo/node_modules/@angular/cli/bin/ng

However:

  "workspaces": {
    "packages": [
      "packages/*"
    ],
    "nohoist": [
      "**/@angular*",
      "**/@angular*/**",
      ...
    ]
  },

I could work around it by adding the above to my root package.json thus disabling hoisting on @angular related packages so they all sit together. ng update then runs properly. The nohoist entries can then be removed after running ng update, since @angular itself works properly and doesn't need nohoist.

The correct fix here would be for @angular/core to add @angular/compiler-cli and typescript to peerDependencies or peerDependenciesMeta, which would also move #16980 further along. Unfortunately, because updates work one version at a time, I'm not sure how to fix this without going back and fixing it in various previous versions of @angular/core.

But even so, the changes in this PR should move this further along.

clydin · 2020-08-28T18:15:19Z

Thank you for the extensive work on improving the update command.
Based on the issues still present after this PR in regards to full workspace support, can you adjust the change to be a refactor that improves update package discovery instead of a feature. The remaining update issues will need to be addressed as well as an analysis of any potential CLI usage and runtime concerns including documentation before yarn workspace support can be considered feature complete. However, these changes definitely provide an improvement.

andreialecu · 2020-08-29T08:35:06Z

@clydin I have updated the PR as per your requirement.

I'd like to also point out something that I think may be a fundamental problem in how ng update currently works and should probably be addressed at some point:

First of all, to illustrate, run this: https://runkit.com/andreialecu/5f4a0a45b7d56e001a469c1e
If you comment out line 18 (require.resolve) the output is entirely different. Make sure you're running on Node 12. On Node 8 (the default) there's different behavior.

This migration script imports rxjs: https://github.com/angular/angular/blob/master/packages/core/schematics/migrations/static-queries/index.ts

As per my previous comment, rxjs has moved its entry point between version 5 and 6. The project being upgraded was on version 5 initially, with the ./Rx.js entry point. Version 6 has a main of ./index.js but also has its own ./Rx.js file that just imports rxjs-compat: https://unpkg.com/rxjs@6.6.2/Rx.js. (Note that you can get the error back by replacing resolve.sync with require.resolve packages/angular/cli/utilities/package-tree.ts)

Now when a multi-version upgrade runs, the following can happen:

if a schematic for a previous version depends on a package such as rxjs and requires it, it will lock down its entry point (and potentially even its entire code via require.cache)
npm/yarn install may update rxjs to a future version, with different behavior
a different schematic may also require rxjs but even though node_modules was updated, it is not guaranteed to be able to load the latest version.

I think the only right solution would be to always start a fresh process after npm/yarn install and run further schematics and migrations in this fresh process. If further clarification is required, let me know.

andreialecu · 2020-08-31T10:20:12Z

The correct fix here would be for @angular/core to add @angular/compiler-cli and typescript to peerDependencies or peerDependenciesMeta, which would also move #16980 further along.

Further reading here: https://yarnpkg.com/advanced/rulebook#packages-should-only-ever-require-what-they-formally-list-in-their-dependencies

clydin · 2020-08-31T12:42:32Z

The correct fix here would be for @angular/core to add @angular/compiler-cli and typescript to peerDependencies or peerDependenciesMeta, which would also move #16980 further along.

This is intentional. The framework libraries are web-only packages that cannot have additional dependencies. Improvements to schematics and migrations are in the planning/design stages that will help to alleviate these type of issues.

merceyz · 2020-08-31T12:45:41Z

framework libraries are web-only packages that cannot have additional dependencies

The thing is they do have additional dependencies they just don't declare it

clydin · 2020-08-31T12:51:31Z

That is correct but that part is the intentional aspect since they are only needed for one specific use case during development and not during runtime or outside the migration scenario of the CLI. It is not an ideal situation but it will be improved moving forward. One of the primary goals of the changes mentioned to schematics/migrations is to remove the situation of the hidden dependencies.

clydin · 2020-09-01T17:08:27Z

packages/angular/cli/lib/init.ts

@@ -20,7 +21,7 @@ const packageJson = require('../package.json');

 function _fromPackageJson(cwd = process.cwd()): SemVer | null {
  do {
-    const packageJsonPath = path.join(cwd, 'node_modules/@angular/cli/package.json');
+    const packageJsonPath = resolve.sync('@angular/cli/package.json', { paths: [process.cwd()] });


Can you remove this change? While this file could use some improvements, it is out of scope for the update related changes in this PR.

@clydin so there is a bug here I introduced, thanks for catching it, it should be paths: [cwd] instead.

However, this change can be related to this PR. Consider the following scenario:

/monorepo/package.json /monorepo/node_modules/@angular/cli/... <- @angular/cli could be hoisted here by yarn (most commonly is, unless the monorepo contains multiple angular projects on different angular versions) /monorepo/packages/webapp/package.json <- depends on @angular/cli /monorepo/packages/webapp/node_modules/... <- other deps, but @angular/cli not stored here

Now, if a user runs ng update while being cwd to /monorepo/packages/webapp/ the logic above will fail to find the locally installed @angular/cli and will use the global one.

ng update will most commonly be run from within /monorepo/packages/webapp/ so this will occur basically every time, it just depends where yarn will hoist @angular/cli (which is not predictable). So it can result in unexpected behavior in some projects, but not others.

Are you sure you want me to revert it?

Please do. That function will not exist soon. The problem demonstrated above will be removed as a result as well.

clydin · 2020-09-01T17:16:07Z

tests/legacy-cli/e2e/utils/assets.ts

@@ -4,7 +4,8 @@ import {getGlobalVariable} from './env';
 import {relative} from 'path';
 import {copyFile, writeFile} from './fs';
 import {useBuiltPackages} from './project';
-import { git, silentNpm } from './process';
+import { git, silentNpm, silentYarn } from './process';
+import { skip } from 'rxjs/operators';


skip doesn't appear to be used.

Got it. Will force push a fix for this once a decision is made on the other comment.

andreialecu · 2020-09-03T07:27:19Z

I'm not sure what happened with the tests. They were previously running fine. Maybe it's a flake?

alan-agius4 · 2020-09-03T08:12:55Z

Master is currently red, we’re working on it 😊
PS: you have a conflict

andreialecu · 2020-09-03T09:41:10Z

@alan-agius4 apologies, somehow I messed up to revert that file properly, hence the conflict. Should be good now I hope.

PS: If you need to do any other minor changes (like variable names and what not) and can't wait for me, "Allow edits from maintainers" is enabled.

clydin · 2020-09-03T14:12:34Z

A rebase against master should fix CI.
Otherwise I think this looks good.

angular-automatic-lock-bot · 2020-10-08T08:08:36Z

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.}

googlebot added the cla: yes label Aug 25, 2020

andreialecu marked this pull request as draft August 25, 2020 14:35

andreialecu force-pushed the feat-ngupdateyarnworkspaces branch 2 times, most recently from 0921dbc to f4df12d Compare August 25, 2020 14:44

andreialecu marked this pull request as ready for review August 25, 2020 15:17

andreialecu marked this pull request as draft August 25, 2020 15:37

andreialecu force-pushed the feat-ngupdateyarnworkspaces branch from f4df12d to 13c5b32 Compare August 25, 2020 19:07

andreialecu marked this pull request as ready for review August 25, 2020 19:43

andreialecu force-pushed the feat-ngupdateyarnworkspaces branch from 13c5b32 to 146d032 Compare August 26, 2020 09:09

andreialecu force-pushed the feat-ngupdateyarnworkspaces branch 5 times, most recently from 404d569 to f16d985 Compare August 27, 2020 16:39

This was referenced Aug 27, 2020

'ng update' does not work with yarn workspaces #14841

Open

Angular CLI and monorepos (like lerna, yarn workspaces) #7097

Closed

andreialecu force-pushed the feat-ngupdateyarnworkspaces branch 3 times, most recently from bbca07c to 6fe5758 Compare August 27, 2020 20:35

clydin added the target: major This PR is targeted for the next major release label Aug 28, 2020

andreialecu force-pushed the feat-ngupdateyarnworkspaces branch from 6fe5758 to f6fc0ee Compare August 29, 2020 07:30

andreialecu changed the title ~~feat(@angular/cli): support yarn workspaces in ng update~~ refactor(@angular/cli): improve update package discovery Aug 29, 2020

andreialecu force-pushed the feat-ngupdateyarnworkspaces branch 2 times, most recently from db3934f to c2c05b3 Compare August 29, 2020 07:41

andreialecu closed this Aug 29, 2020

andreialecu reopened this Aug 29, 2020

andreialecu mentioned this pull request Aug 29, 2020

regression: require.resolve() caches package.json when it shouldn't nodejs/node#34967

Open

alan-agius4 requested a review from clydin September 1, 2020 16:20

clydin requested changes Sep 1, 2020

View reviewed changes

andreialecu force-pushed the feat-ngupdateyarnworkspaces branch from c2c05b3 to 7840849 Compare September 3, 2020 06:04

refactor(@angular/cli): improve update package discovery

dc63565

andreialecu force-pushed the feat-ngupdateyarnworkspaces branch from 7840849 to dc63565 Compare September 3, 2020 09:39

clydin approved these changes Sep 3, 2020

View reviewed changes

alan-agius4 added the action: merge The PR is ready for merge by the caretaker label Sep 3, 2020

angular deleted a comment from ngbot bot Sep 3, 2020

alan-agius4 merged commit d01d647 into angular:master Sep 3, 2020

andreialecu deleted the feat-ngupdateyarnworkspaces branch September 7, 2020 08:27

andreialecu mentioned this pull request Sep 25, 2020

Trying to use the plugin with limited success bgotink/yarn-plugin-angular#1

Closed

angular-automatic-lock-bot bot locked and limited conversation to collaborators Oct 8, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(@angular/cli): improve update package discovery #18610

refactor(@angular/cli): improve update package discovery #18610

andreialecu commented Aug 25, 2020 •

edited by alan-agius4

Loading

andreialecu commented Aug 27, 2020 •

edited

Loading

andreialecu commented Aug 27, 2020 •

edited

Loading

andreialecu commented Aug 28, 2020 •

edited

Loading

clydin commented Aug 28, 2020

andreialecu commented Aug 29, 2020

andreialecu commented Aug 31, 2020

clydin commented Aug 31, 2020

merceyz commented Aug 31, 2020

clydin commented Aug 31, 2020 •

edited

Loading

clydin Sep 1, 2020

andreialecu Sep 2, 2020 •

edited

Loading

clydin Sep 2, 2020

clydin Sep 1, 2020

andreialecu Sep 2, 2020

andreialecu commented Sep 3, 2020

alan-agius4 commented Sep 3, 2020

andreialecu commented Sep 3, 2020 •

edited

Loading

clydin commented Sep 3, 2020

angular-automatic-lock-bot bot commented Oct 8, 2020

refactor(@angular/cli): improve update package discovery #18610

refactor(@angular/cli): improve update package discovery #18610

Conversation

andreialecu commented Aug 25, 2020 • edited by alan-agius4 Loading

andreialecu commented Aug 27, 2020 • edited Loading

andreialecu commented Aug 27, 2020 • edited Loading

andreialecu commented Aug 28, 2020 • edited Loading

clydin commented Aug 28, 2020

andreialecu commented Aug 29, 2020

andreialecu commented Aug 31, 2020

clydin commented Aug 31, 2020

merceyz commented Aug 31, 2020

clydin commented Aug 31, 2020 • edited Loading

clydin Sep 1, 2020

Choose a reason for hiding this comment

andreialecu Sep 2, 2020 • edited Loading

Choose a reason for hiding this comment

clydin Sep 2, 2020

Choose a reason for hiding this comment

clydin Sep 1, 2020

Choose a reason for hiding this comment

andreialecu Sep 2, 2020

Choose a reason for hiding this comment

andreialecu commented Sep 3, 2020

alan-agius4 commented Sep 3, 2020

andreialecu commented Sep 3, 2020 • edited Loading

clydin commented Sep 3, 2020

angular-automatic-lock-bot bot commented Oct 8, 2020

andreialecu commented Aug 25, 2020 •

edited by alan-agius4

Loading

andreialecu commented Aug 27, 2020 •

edited

Loading

andreialecu commented Aug 27, 2020 •

edited

Loading

andreialecu commented Aug 28, 2020 •

edited

Loading

clydin commented Aug 31, 2020 •

edited

Loading

andreialecu Sep 2, 2020 •

edited

Loading

andreialecu commented Sep 3, 2020 •

edited

Loading