rushstack icon indicating copy to clipboard operation
rushstack copied to clipboard

[rush] Let's get NPM 6 working with Rush

Open octogonz opened this issue 7 years ago • 32 comments
trafficstars

Rush shouldn't be in a position of recommending that people use the ancient NPM 4. Due to recent interest, I am opening this meta-issue to track the punch list of work items needed to get the latest NPM version to be supported by Rush.

Off the top of my head:

  • We need to a workaround for the NPM's regression with installing "file:" version specifiers (See https://github.com/Microsoft/web-build-tools/issues/708#issuecomment-399013210). For now, Rush should wipe NPM's entire (local) cache on every install. Later we'll improve this by deleting only the @rush-temp entries, or (ideally) by getting NPM to fix the root cause.
  • We need to eliminate Rush's reliance on the npm shrinkwrap command, because in NPM 5.x it corrupts the shrinkwrap file. (NPM 4.x and NPM 6.x seem to be okay)
  • We should figure out a better solution for the "version" field problem currently being solved by Rush's _fixupNpm5Regression()
  • Last I checked, read-package-tree had some other problems NPM 6 node_modules folders; some fixes may be required there
  • NPM 6 apparently still relies on the request library that has the race condition. Maybe we should document the known workarounds.

octogonz avatar Oct 17 '18 20:10 octogonz

@kenotron @scsewall @jbcpollak

octogonz avatar Oct 17 '18 22:10 octogonz

@pgonzal can I suggest you coordinate with @zkat?

jbcpollak avatar Oct 19 '18 17:10 jbcpollak

Sure! @zkat this would be the absolute most valuable issue for NPM to help us out with:

  • We need to a workaround for the NPM's regression with installing "file:" version specifiers (See https://github.com/Microsoft/web-build-tools/issues/708#issuecomment-399013210). For now, Rush should wipe NPM's entire (local) cache on every install. Later we'll improve this by deleting only the @rush-temp entries, or (ideally) by getting NPM to fix the root cause.

I can provide an isolated repro if needed. But the basic idea is that when we do this...

package.json

{
  "name": "rush-common",
  "description": "Temporary file generated by the Rush tool",
  "private": true,
  "version": "0.0.0",
  "dependencies": {
    "@rush-temp/app1": "file:./projects/app1.tgz"
  }
}

...after npm install has run once, if the "app1.tgz" file is updated by Rush, thereafter npm install seems to always install the old tarball. The only way to make it recognize the change is EITHER:

  1. Delete both node_modules AND package-lock.json (which defeats the point of an incremental update), OR
  2. Delete node_modules AND run npm cache clean --force (which is terrible performance)

Anything less than that, and NPM continues to install the old tarball from the cache, ignoring our updates. This more or less was working in NPM 4, but repros in both NPM 5 and NPM 6.

octogonz avatar Oct 19 '18 17:10 octogonz

When I tried workaround #2 again just now with the latest NPM 6.4.1, it's crashing with this call stack:

Unhandled rejection Error: invalid config key requested: errors
    at BadKeyError (C:\Users\pgonzal\AppData\Roaming\nvm\v8.10.0\node_modules\npm\node_modules\figgy-pudding\index.js:93:23)
    at pudGet (C:\Users\pgonzal\AppData\Roaming\nvm\v8.10.0\node_modules\npm\node_modules\figgy-pudding\index.js:101:5)
    at FiggyPudding.get (C:\Users\pgonzal\AppData\Roaming\nvm\v8.10.0\node_modules\npm\node_modules\figgy-pudding\index.js:27:12)
    at Object.get (C:\Users\pgonzal\AppData\Roaming\nvm\v8.10.0\node_modules\npm\node_modules\figgy-pudding\index.js:159:16)
    at Object.checkData (C:\Users\pgonzal\AppData\Roaming\nvm\v8.10.0\node_modules\npm\node_modules\ssri\index.js:232:22)
    at write (C:\Users\pgonzal\AppData\Roaming\nvm\v8.10.0\node_modules\npm\node_modules\cacache\lib\content\write.js:34:31)

If I also delete package-lock.json then this error goes away.

octogonz avatar Oct 19 '18 17:10 octogonz

I also ran into that during my investigations. For me, I think that it was caused by NPM 6 encountering a package-lock.json generated by NPM 5. I had to delete npm-shrinkwrap.json to get past it.

scsewall avatar Oct 19 '18 18:10 scsewall

In this case the package-lock.json was created by the same version of NPM. (This was an isolated manual repro, not a real Rush installation.)

octogonz avatar Oct 19 '18 19:10 octogonz

hey, y'all! I'd love to help out here with any questions y'all have (though I've got pretty low availability lately). One thing to note: npm is moving away from request entirely with the next release, so that one will at least be addressed.

I'll also note that I don't believe anything having issues with npm@5 is worth fixing: we no longer support that version, and we won't be doing any additional releases in the 5.x line unless there's a very major security issue.

I'm curious what the read-package-tree issue is -- this is another thing we're planning on rewriting very soon, so knowing what we need to watch out for will be useful.

zkat avatar Oct 26 '18 16:10 zkat

hey, y'all! I'd love to help out here with any questions y'all have (though I've got pretty low availability lately). One thing to note: npm is moving away from request entirely with the next release, so that one will at least be addressed.

That's really great! It should really improve reliability of NPM under heavy usage.

@zkat what about the "file:" version specifiers issue that I mentioned above? That one really hurts performance for NPM. It's definitely our biggest need.

octogonz avatar Oct 26 '18 17:10 octogonz

I'll also note that I don't believe anything having issues with npm@5 is worth fixing: we no longer support that version, and we won't be doing any additional releases in the 5.x line unless there's a very major security issue.

That's fine. The issues I called out at the top of this issue all affect the latest NPM. If we can get the latest NPM working, that should be sufficient for most people's needs.

octogonz avatar Oct 26 '18 19:10 octogonz

In the meantime, is it best practice to use PNPM? Thanks!

abirmingham avatar Aug 05 '19 19:08 abirmingham

We don't really recommend one of the package managers as being "best". The decision depends on your needs. But I believe the Rush docs call out some non-subjective considerations:

  • NPM has known compatibility issues with Rush, as detailed in this thread. We want to get that fixed, but there doesn't seem to be a lot of community interest in it, not even for fixing the bugs that were identified in NPM itself.
  • Classic Yarn uses the same installation model as NPM, which suffers from the phantom/doppelganger issues that are solved by PNPM's symlinking algorithm
  • Yarn "Plug 'n Play" has similar benefits as PNPM, but this mode is relatively new, and Rush doesn't support it yet. (We'd welcome a PR for that!)
  • If you choose PNPM, certain older libraries may encounter compatibility issues because they aren't honoring the NodeJS spec, and instead rely on quirks of NPM. Most of these issues have been solved by now, but there are still a few stragglers.

Fortunately, switching package managers is pretty easy with Rush, and rush install almost completely hides these details from developers working in your repo.

octogonz avatar Aug 05 '19 21:08 octogonz

I'm experiencing this error while using my company's proxy (while using proper syntax like ^1.2.3). For now I've rolled back to yarn, which is working, but I hope this will be fixed someday.

Jabher avatar Aug 24 '19 14:08 Jabher

Status update: NPM recently announced that version 7 will finally provide support for monorepos. Today, the monorepo support implemented by Yarn and PNPM is already reasonably close to what we do in rush install and rush link. Thus, we're considering an overhaul of Rush that would delegate most of the installation/linking operations to the underlying package manager. The Rush maintainers currently spend a lot of time investigating/maintaining issues with installation edge cases, so this would offload a lot of that, freeing us up to focus on more differentiating features such as sharded builds, multi-project watch, publishing workflows, etc.

One downside is that NPM and Yarn classic will lose Rush's protection against phantom dependencies. But we've found that users with complex installs inevitably move to PNPM or Yarn Plug'n'Play anyway. But an upside is that you would be able to use the package manager's monorepo commands in your Rush repo.

octogonz avatar Aug 29 '19 01:08 octogonz

Awesome direction, @octogonz. Do you have another tracking issue on this? Would love see more details on what it would mean to have workspaces support with rush

kenotron avatar Aug 29 '19 03:08 kenotron

I have 2 projects one with vue-cli and one with webpack. when i installed through:

  • pnpm 3.8 i found errors: https://github.com/vuejs/vue-cli/issues/4571 https://github.com/pnpm/pnpm/issues/1678#issuecomment-469981972 https://github.com/pnpm/pnpm/issues/1678 https://github.com/webpack/webpack/issues/5087 and etc
  • yarn 1.10.1 i found errors: https://github.com/yarnpkg/yarn/issues/3967 and error in subproject on build looks like "can not resolve subproject/node_modules/@rush-temp/my-project/node_modules/webpack" and etc
  • npm 6.9 i found too many errors like:
npm WARN tarball tarball data for @rush-temp/my-project@file:projects\my-project.tgz (sha512-efOjexF3ji60ua49SHlPR1f1QU2cwUYRZiaFVxf0YM2vVj8hFSi8usX0UxTXkkOt3d2p3QUy0+tAn253d2mQ8w==) seems to be corrupted. Trying one more time.
npm ERR! cb() never called!

and etc

I spent a lot of time to earn at least something, but nothing worked. With pnpm, you simply cannot build a vue-cli project. With yarn, you cannot resolve dependencies of a dependent project. With npm do not install dependent project.

NikitaIT avatar Sep 14 '19 22:09 NikitaIT

NPM was my last hope and I left it here

NikitaIT avatar Sep 14 '19 22:09 NikitaIT

@NikitaIT Could you share a repro of your build that failed using PNPM?

I've heard from several people that vue-cli does not correctly declare its imports, making it incompatible with PNPM and Yarn Plug'n'Play. I remember seeing that their Troubleshooting page says "It is because webpack resolves symlinks to their real locations by default, thus breaks ESLint / Babel config lookup". This suggests that the Vue maintainers don't understand that Vue itself is the cause of this trouble. I investigated it a while ago, but since I don't use Vue myself, I simply followed their tutorial and was unable to repro the error.

If you could share a PNPM repro, I'd be willing to debug it, determine whether the problem is really Vue, and if so open a proper issue to help get this solved. As stated above, Rush does have plans this fall to enable NPM/Yarn to use their conventional ("shamefully flatten") installation strategy, which should solve your problem. But that's not a scalable way of installing packages (due the phantom/doppelganger concerns), so this "fix" wouldn't help larger monorepos that really do need a PNPM or Yarn Plug'n'Play installation strategy.

octogonz avatar Sep 15 '19 22:09 octogonz

@octogonz Repo: rush pnpm by default + vue-cli(dart-sass, babel, typescript) by default

rush init 
vue create vue-cli-3-ts-project
// add vue-cli-3-ts-project to rush.json
rush update
rush install
rush build
// oops!
// run with 
rush-vue-cli\common\temp\pnpm-local\node_modules\pnpm\lib\bin\pnpm.js run serve --scripts-prepend-node-path=auto

Init only: https://github.com/NikitaIT/rush-vue-cli

  • Failed to resolve loader: cache-loader // I know that it will be for all loaders, even if fix it for cache-loader
  • 36:1 This syntax requires an imported helper but module 'tslib' cannot be found

Symbolic Links in node_modules(Troubleshooting): https://github.com/NikitaIT/rush-vue-cli/tree/off-resoleve-symlinks

  • same errors, nothing happened, expectedly

Win 10, node v10.16.3

NikitaIT avatar Sep 22 '19 00:09 NikitaIT

BTW I've opened this issue to track the feature to enable Rush to use the package manager's monorepo support (workspaces): https://github.com/microsoft/rushstack/issues/1553

octogonz avatar Sep 30 '19 17:09 octogonz

Hey @octogonz has any progress been made on this?

ChocPanda avatar Oct 28 '19 21:10 ChocPanda

It's next in the queue in our roadmap.

octogonz avatar Nov 04 '19 22:11 octogonz

Any movement on this? I am getting some serious npm errors in my pipeline due to corrupted package.tgz files. I am hoping this fixes that issue.

jblevins1991 avatar Dec 15 '19 03:12 jblevins1991

If you choose NPM, you may need to use an older release. NPM 5.x and 6.x are both known to have unresolved regressions that cause trouble in Rush repos. NPM 4.5.0 is the most recent version that's known to work very reliably, but unfortunately it's pretty old. (We'd greatly appreciate community help improving this situation. We're using GitHub issue #886 to track this effort.)

Ok, if NPM 5 and 6 are no good, how about NPM 7 and 8? Really surprised to see the docs so far behind on this issue. And this issue hasn't been updated in 2+ years. Feel like I am missing something rather obvious?

oravecz avatar Mar 15 '22 00:03 oravecz

any updates on this? I plan to move from yarn because of this issue https://github.com/microsoft/rushstack/issues/1748

and pnpm is not possible, since it breaks some build on my apps

fahmifan avatar Aug 10 '23 02:08 fahmifan

@fahmifan - We encountered a number of issues with NPM over the years, so supporting it hasn't been a priority. We'd happily take a PR to update support for NPM, but our team likely won't put that together ourselves.

What issues are you hitting with pnpm?

iclanton avatar Aug 23 '23 18:08 iclanton

Today the important differences are not the package manager itself, but rather the underlying installation model. The Lockfile Explorer docs summarize the reasons why NPM's installation model is not a good tech bet for a monorepo, regardless of whether you are using Yarn Classic or NPM.

and pnpm is not possible, since it breaks some build on my apps

From my experience, there is almost always a way to get PNPM working, and although it can be nontrivial work, it generally turns out to be less work than other approaches such as trying to run your monorepo with Yarn or NPM. The method for fixing PNPM incompatibilities generally involves .pnpmfile.cjs overrides, or else a small patch to a poorly behaved tool so that its module resolver correctly handles symlinks. For newer PNPM versions, Rush now supports rush-pnpm patch which allows patching to happen during installation, avoiding the need to modify or fork the upstream project.

octogonz avatar Aug 24 '23 02:08 octogonz

@iclanton I have issue when running our FE app built with quasar v1 locally, the error is:

This dependency was not found
$WORKSPACE/$PROJECT/quasar/client-entry.js in multi $PROJECT/common/temp/node_modules/.pnpm/[email protected][email protected]/node_modules/webpack-dev-server/client?http://0.0.0.0:8080 (webpack)/hot/dev-server.js ./.quasar/client-entry.js

I think maybe it's caused by babel-loader, so I tried pnpm shamefully-hoist, got different error

fahmifan avatar Aug 29 '23 04:08 fahmifan

I have been moving all my packages in a monorepo with rush and pnpm for some months with great success. However, i found out, that there are two (big) environments that end up being problematic (near impossible ?) with pnpm :

  • electron
  • react native

vjau avatar Dec 23 '23 16:12 vjau

:wave: This issue mentions npm 6, but we are currently (as of Node.js 20) on npm 10. Is this still an issue, or something that was missed in a prioritization/close cycle?

drazisil avatar Apr 02 '24 23:04 drazisil

Yes, we should probably close this ticket. NPM's installation model is not a good fit for large code bases, and the compatibility issues with PNPM are now fully solved, so it is very hard to find anyone willing to work on NPM support for Rush.

You can find some background here: https://lfx.rushstack.io/pages/concepts/install_models/

octogonz avatar Apr 04 '24 02:04 octogonz