search

microsoft / rushstack

Monorepo for tools developed by the Rush Stack community
https://rushstack.io/
Other
5.74k stars 587 forks source link

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

Open octogonz opened 5 years ago

octogonz commented 5 years ago

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:

octogonz commented 5 years ago

@kenotron @scsewall @jbcpollak

jbcpollak commented 5 years ago

@pgonzal can I suggest you coordinate with @zkat?

octogonz commented 5 years ago

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

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 commented 5 years ago

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.

scsewall commented 5 years ago

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.

octogonz commented 5 years ago

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

zkat commented 5 years ago

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.

octogonz commented 5 years ago

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 commented 5 years ago

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.

abirmingham commented 4 years ago

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

octogonz commented 4 years ago

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:

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

Jabher commented 4 years ago

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.

octogonz commented 4 years ago

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.

kenotron commented 4 years ago

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

NikitaIT commented 4 years ago

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

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 commented 4 years ago

NPM was my last hope and I left it here

octogonz commented 4 years ago

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

NikitaIT commented 4 years ago

@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

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

Win 10, node v10.16.3

octogonz commented 4 years ago

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

ChocPanda commented 4 years ago

Hey @octogonz has any progress been made on this?

octogonz commented 4 years ago

It's next in the queue in our roadmap.

jblevins1991 commented 4 years ago

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.

oravecz commented 2 years ago

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?

fahmifan commented 11 months ago

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

iclanton commented 10 months ago

@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?

octogonz commented 10 months ago

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.

fahmifan commented 10 months ago

@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/webpack-dev-server@3.11.3_webpack@4.46.0/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

vjau commented 6 months ago

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 :

drazisil commented 3 months ago

: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?

octogonz commented 3 months ago

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/

iclanton commented 3 months ago

@octogonz - you want to just go ahead and close this issue?

Maybe before we close this we should add a feature to Rush test prints an explanation for our minimal support for NPM.

bijela-gora commented 2 months ago

Just wondering how many users are using Rush with npm?