Conversation
We have hardcoded npm function calls here, this adjusts it to support PNPM mode with MONOREPO_TOOLS_USE_PNPM
| if (packageLockOnly) { | ||
| // npm uses --package-lock-only, pnpm uses --lockfile-only | ||
| args.push( | ||
| packageManager === 'pnpm' ? '--lockfile-only' : '--package-lock-only', |
There was a problem hiding this comment.
Do we need a pnpm-lock.yaml to align behavior locally and on CI? And if we do add pnpm-lock do we have a way of keeping it in sync with package-lock.json?
There was a problem hiding this comment.
if a repo uses pnpm, then we will only have pnpm-lock, we should not have package-lock.
One can also set pnpm as default package manager in package.json so then even if someone runs npm i it'll redirect to pnpm
There was a problem hiding this comment.
These tools could try to look for pnpm-lock to determine what mode to run in maybe but pnpm lock might be at a higher level folder etc so env just seems more flexible
There was a problem hiding this comment.
Understood but this PR is for adding optional use of pnpm, so there is no pnpm-lock file, as a result is anyone who uses pnpm going to suffer from a broken experience at least locally when they go to install and they get a very different set of transient dependencies than are used on CI? (as a comparable example, I often accidentally work in compass on the wrong npm version and even that deviation causes a change in the result of npm i and tests fail)
Main concern here, sure we exec pnpm, but is it useful? (will it be useful? because it may work today but when it breaks what do we do about it to fix it, what will be the urgency to fix it, etc.)
|
Seems like Ubuntu runner was blocked by coveralls... Tests succeeded otherwise. |
|
Before implementing pnpm everywhere, which is a pretty important change, if what we want is better performance we should first try npm with the "linked" install strategy, which uses the same package-lock.json but it is faster, as mimicks how pnpm works internally. |
|
@kmruiz I don't disagree, it's worth trying; I was originally misled and thought we had a consensus on interest in pnpm. That said this PR doesn't adopt it on its own, just makes it possible to at least run experiments in the repos which use our monorepo tools. We can try npm as well but it'd still be worth having something to compare to, so unblocking that effort to be able to experiment. On a more general level though,
|
|
Gaining consensus on something like this is usually through a scope and design so we can account for the impact and we can plan time to dedicate to the effort it takes to migrate. You make great points about pnpm's design and Kevin's suggestion for trying npm's install-strategy should be accounted for in the design of either a migration or dual package manager support. Sorry if you were misled! I think the situation is that folks have generally been somewhat unprompted about this issue. The vscode and mcp-server adoption of pnpm didn't impact me so I had no stake in the matter one way or the other. I think we're finally crossing the boundary into needing more stakeholders to share their input on this matter and we should open that forum, we can start simply with a slack thread but I think there will be value in planning this out in a document. If this PR enables optional use of pnpm and that use won't lead to difficulties with the lockfile or other parts of the tooling then I think it may be fine to keep this train rolling. 🙂 |
|
It's a matter of consistency, this repository is used across all our tooling and every single inconsistency across npm and pnpm will just spread around our repositories. pnpm handles dependency resolution differently than npm, and if at some point we need overrides, we need to maintain them for both npm and pnpm. For libraries, we don't publish the package-lock.json as it's the ending product which should be able to override and define it's own dependencies.
If that's the point, have you tried the
The problem is not across pnpm versions, is about npm and pnpm. We are not going to change mongosh for example in the short term, and we mainly use npm everywhere. And we in general don't commit the package-lock.json on libraries.
We already use lerna and workspaces across many products successfully, and while I'm sure it can be improved in several ways, the consequences of adopting pnpm in this repository might be far more detrimental to the stability of our overall portfolio than the possible benefits of simplified workspace resolution.
I do agree! But we should start with a problem statement. What are we trying to solve? We've rushed to adopt pnpm in some of our products without consensus, and while they are end products so the impact is low (like the mcp server) we've already had problems with vscode because we have a test suite in mongosh for testing the integration. While the issue with vscode+mongosh is solved, we've had the CI broken for months because at the end, maintaining two dependency management systems has a cost. We should be more careful adopting technologies, because while pnpm might look good, there has been many forks and alternatives to npm in the past that are now deprecated or abandoned, and while npm might not be the best (and I can assure that I don't precisely love it), it's the standard and is pretty well suited to what we do. |
Yeah this requires explicit opt-in through an environment variable to even get to PNPM mode, so it's not like this library will suddenly start enforcing PNPM or a different lockfile. In the current reality, monorepo tools cannot be used in vscode or MongoDB MCP Server. We aren't using them so it doesn't matter, but as far as it goes it doesn't make the status quo of mongosh / Compass usage of npm any different.
I have fixed this test_vscode many times, it breaks often and from many subtle changes to the vscode and its CI, so pnpm is anything that different there. I had many more issues with npm package-lock resolutions. Granted, the projects with pnpm I have used have been simpler, so it's not necessarily better.
It's a great question but I believe sometimes an experiment doesn't need an answer to this. Since pnpm is also adopted by the cloud org (and half of our repos, but that's more arguable), it was also my impression that we're way past more core discussions about whether pnpm carries a deprecation risk. On that front, I can't say I'm more sold on stability of a NPM mode whose best documentation is an RFC prooposal. That all said, this was mainly driven by curiosity and desire to do something interesting on the side while I'm doing some more routine tasks rather than any strong desire to push this, especially since I did not realize people were not that sold on adopting it. This was meant to isolated in a experiment PR mongodb-js/mongosh#2682 but I admit this stopped being an experiment with this PR which was actually looking to be merged. So I will pause my effort in that direction for now, it was reaching the limit I was going to bother testing it out with anyways. We can revisit this PR and other changes afterwards 🙂 |
We have hardcoded npm function calls here, this adjusts it to support PNPM mode with
MONOREPO_TOOLS_USE_PNPM=true.