RFD 177: Upgrade JS package manager #43404
Conversation
|
The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with |
gzdunek
added
the
no-changelog
There was a problem hiding this comment.
Could you look at how pnpm interacts with dependabot? That's something we need to keep in mind now.
There was a problem hiding this comment.
From what I see, we don't need to change anything, pnpm uses the same value for package-ecosystem: npm.
There was a problem hiding this comment.
Dependabot seems to support only pnpm v7 and v8, at least according to the docs. There are some reports of v9 working though. dependabot/dependabot-core#9522
There was a problem hiding this comment.
Oops, I forgot to send the comment.
People report that pnpm v9 works when "packageManager": "[email protected]" is set in package.json. We will set this anyway, so we shouldn't run into this problem.
There was a problem hiding this comment.
How about Renovate? We may switch from Dependabot to Renovate in the future..
| 3. Project-wide tools used in the root `package.json` should be kept in | ||
| `build/package.json`, if possible. | ||
| Example: `jest` or `storybook` can be installed in `build/package.json` | ||
| which will expose them through scripts to the root `package.json`. |
There was a problem hiding this comment.
What's the purpose behind a separate build package then?
I believe the package structure that we have today comes from Gravity and Teleport which had to share common deps. Gravity got discontinued, but the package structure remains, because we now have Connect which also needs the same deps as Teleport.
The other question is, do we need workspaces at all?
Where are workspaces useful?
From my experience, they're useful when inside a single repo you work on multiple individual packages that are consumed separately, and those packages depend on a bunch of smaller packages that are in the repo. "Consumed separately" might mean they're published as separate npm packages. Or there's multiple apps and separate CI jobs want to pull in only deps required by a certain app.
When working on those separate packages, you don't want to be limited by the regular npm package limitations. I.e., if you want to update <Button>, it's best if you could just update one file and both teleterm and teleport picked up the changes immediately. It'd be bad if you had to release [email protected] first and then update it in both teleterm and teleport.
Perhaps also those separate packages need separate deps, or separate versions of deps.
Quoting from Yarn docs:
Workspaces shine in many situations, with the main ones being:
When a project has a core package surrounded with various add-ons; this is for example the case for Babel.
When you need to publish multiple packages and want to avoid your contributors having to open PRs on many separate repositories whenever they want to make a change. This is for example the case for Jest.
Or when projects want to keep strict boundaries within their code and avoid becoming an entangled monolith. This is for example the case for Yarn itself, or many enterprise codebases.
The last point might sound as if it applied to us.
How do we use workspaces at the moment?
Currently, we do none of that. We always install all deps, we always clone the whole repo. There's pretty much no dep separation. Almost all dep management is done by Vite which just pulls relevant deps into the final bundle.
On the contrary – we want both projects (Teleport and Connect) to use the same versions of deps so that it's easy to reuse code between them. So the way pnpm resolves deps is beneficial here – we want the same deps always, everywhere.
The way we use workspaces feels similar to how you'd use regular CommonJS modules to share code. <Button> is used in multiple places, so you place it in button/index.js and use it in multiple places. vite is used by both projects, so you place it in build/package.json and use it in multiple places.
But as you mentioned, that's not how packages work in. The closest we are to using packages the "correct" way is shared and design maybe, where I don't really care how <Button> is implemented or what deps it needs, I just want to use it in my component. So I import Button from design and yarn handles design deps during installation.
Could we live without workspaces?
Connect needs a separate package.json so that electron-builder can pick correct deps. This is the one place where dep separation actually matters. Otherwise each .dmg with Connect would ship with JS deps it doesn't use at all.
Could we build Connect without workspaces though? What if it just continued to be a separate folder? Could we survive not using workspaces or are there are benefits to them that I failed to notice?
There was a problem hiding this comment.
It seems that if Connect is supposed to stay consumed as a separate entity by electron-builder, it needs to behave as a real package. As in, it needs to have its own package.json and the resolved packages need to be accounted for in some lockfile.
So maybe the structure proposed by Grzegorz is fine? A separate build package certainly helps with encapsulating which deps are needed for dev purposes only.
There was a problem hiding this comment.
Could we build Connect without workspaces though? What if it just continued to be a separate folder?
When it comes to electron-builder, we could probably use the two package.json structure https://www.electron.build/tutorials/two-package-structure.html?
So in the teleterm/package.json we would still keep the dependencies that should be bundled.
The drawback of it is that we would have to add postinstall script to trigger installing them:
To ensure your dependencies are always updated based on both files, simply add "postinstall": "electron-builder install-app-deps" to your development package.json. This will basically automatically trigger an npm install within your app directory so you don’t have to do this work every time you install/update your dependencies.
But tbh, I'm not a huge fun of this structure, it feels more complicated than the workspace option.
When it comes to workspaces in general, I think the biggest benefit for us is this one:
Or when projects want to keep strict boundaries within their code and avoid becoming an entangled monolith. This is for example the case for Yarn itself, or many enterprise codebases.
Currently we break these boundaries by importing from teleport to shared or to teleterm.
However, while working on the RFD, I realized that we could easily enforce strict rules between the workspaces by getting rid of TS aliases and importing the code directly from the packages, for example @gravitational/shared/AccessRequests instead of shared/AccessRequests. Then we would have to set hoist-workspace-packages=false in the config to prevent linking all workspaces to the root node_modules.
After that, a given workspace would be able to import code only from the workspaces that it declares in its package.json.
Maybe it's possible to achieve a similar result with a linter rule? But for sure enforcing this at the package level provides better DX than during lining.
There was a problem hiding this comment.
What's the purpose behind a separate build package then?
I wasn't 100% sure if we should keep it, after we fully migrate to Vite, we will only have jest and vite configs there, plus the dependencies. Mainly, I wanted to not make too many changes in this migration.
However, maybe it will be better to keep the dev dependencies in the root package.json (some of them will need to be moved there anyway and packages/build will shrink even more).
There was a problem hiding this comment.
I'm not sure how much of it applies to pnpm, but when I was reading about Plug'n'Play, I found this:
Yarn prevents ghost dependencies in the packages your project depends on, but also in your own code itself - this is to decrease the chances that a package would work on your development machine but break once published.
It however has a side effect when it comes to bins. If you have typescript listed at the root of your project, the tsc binary will be available in the root package but only in the root project. In other words, any workspace using the tsc binary in its scripts will need to declare it in its dependencies.
A good recommendation to avoid this kind of issue is to have a "tooling" workspace to contain your infrastructure tools and scripts, and have all other workspaces depend on it.
There was a problem hiding this comment.
I tested this scenario in pnpm and adding typescript to the root package.json makes tsc available in a child workspace, so we wouldn't have this problem.
But I think I will still try to keep the tooling in packages/build and see how it works.
| Yarn also shows a warning `The pnpm linker doesn't support providing different | ||
| versions to workspaces' peer dependencies` during the installation, but I'm not | ||
| sure what significance this has. |
There was a problem hiding this comment.
I think this means a situation where one package has something like react@^17.0.0 in its peer deps and another package has react@^18.0.0. Does this mean that pnpm works in a similar manner to cargo, bundler and other sensible package managers? I didn't spend a lot of time looking for this but I couldn't find how pnpm resolves versions of deps.
There was a problem hiding this comment.
I think this article answers the question https://pnpm.io/how-peers-are-resolved
Does this mean that pnpm works in a similar manner to cargo, bundler and other sensible package managers?
I don't have much experience with other package managers, but I'm guessing you mean resolving exact dependency versions for packages?
So, if I'm correct, Yarn in pnpm mode is not able to resolve different react versions from your example, but pnpm manager can do so. That's another point for pnpm :)
There was a problem hiding this comment.
The proposed plan sounds good to me.
At the bare minimum, it's going to fix the problem with hoisting dependencies. It's going to enforce consistent package versions across projects and force us to be careful with cross-package imports (e.g. importing teleport to shared).
Plug'n'Play seems like the future to me, I think that's how most dependency managers work in other langs. While it's not pnpm's default, it has support for it. And Electron doesn't seem to work well with Plug'n'Play anyway.
My main worries are dependabot support and compatibility with all the different platforms we build our software on.
There was a problem hiding this comment.
Dependabot seems to support only pnpm v7 and v8, at least according to the docs. There are some reports of v9 working though. dependabot/dependabot-core#9522
There was a problem hiding this comment.
Another thing that worries me is compatibility with different platforms that we build our software on. As in, pnpm not working on some old CentOS or whatever.
Before setting out to fix all the problems in the code to make it compatible with pnpm, what do you think about making a tag build with pnpm installing some simple package.json in a separate folder?
IIRC, we use corepack on many platforms, so hopefully installing pnpm will be straightforward.
There was a problem hiding this comment.
Yeah, installing pnpm should be straightforward. We install yarn by calling corepack enable yarn, it needs to be changed to corepack enable to enable Corepack itself, allowing it to install other package mangers.
As for system compatibility, if the buildbox can run modern Node.js, it shouldn't have problems running pnpm (fyi we no longer run Node.js on CentOS). But I see your point, I will run a tag build to test it.
| The ergonomics of this solution are not ideal, but the only other option is to | ||
| move these dependencies to the root `package.json`. |
There was a problem hiding this comment.
Seems simpler to keep it in the root package.json
It's only a handful of libraries and it means we don't have a confusing difference between should it be imported @gravitational/e-imports/ or not, for the sake of keeping some dependencies in a different JSON file
There was a problem hiding this comment.
I think you're right, I wanted to avoid polluting root package.json with unneeded dependencies, but re-exporting the packages from e-imports may be too cumbersome.
And as you said, there shouldn't be too many libraries used exclusively by teleport.e.
I will update the RFD.
There was a problem hiding this comment.
I don't like the idea of putting package-specific deps in the root, but I also don't see a good way of addressing all of our concerns at the same time. 🤔
If we put deps for a specific package in the root package.json, what stops us from just putting all of them there? It'd be easy for some to put teleport or teleterm specific packages in there as well. From their perspective, all they would see is that they had followed an established pattern. There's no easy way to detect that and we'd need to enforce it manually.
Using e-imports would actually enforce that all enterprise-only deps are added to the OSS first and then consumed in ent. It also eliminates the possibility of things getting out of sync. But exposing deps as @gravitational/e-imports/foo certainly adds a bunch of work whenever someone wants to add a dep. How would we even do that? Require someone to set up foo/index.ts in e-imports? idk if there's an easier way.
We could also duplicate ent-only deps in both teleport and teleport.e. That would make them easier to consume. But then we make it easy to update only one of the package.jsons.
Do you have any other ideas?
There was a problem hiding this comment.
How would we even do that? Require someone to set up
foo/index.tsin e-imports? idk if there's an easier way.
Yeah, something like that.
I agree with everything you wrote here. I'm not super happy with any of these options, unfortunately I couldn't find anything better.
But I will spend some more time tomorrow researching it, maybe I will find something!
There was a problem hiding this comment.
Actually, there is one more thing that comes to my mind: having a separate pnpm-lock.yaml in each workspace.
The config option is called shared-workspace-lockfile.
I think it would provide the best DX when it comes to e dependencies - if you need a new package, you don't have to touch OSS at all. We would get rid of e-imports completely.
Also, IMO it's easier to review changes in a smaller lockfile, rather than in a one having 30K+ lines.
But there are some disadvantages too:
- slower installation
- dependabot, we would have to add an entry for JS deps in
e(and I'm not sure if OSS would work with no changes)
It seems to me that it would give us the best of both worlds (strictness and good developer experience), but OTOH I'm a bit afraid of making our setup more complex.
There was a problem hiding this comment.
slower installation
How much slower? I think we can afford a bit more time for the sake of developer experience
I think this is a good option and we should try it out, as long as it doesn't slow us down too much. If it isn't great, we can try other methods
There was a problem hiding this comment.
Here are some quick tests. The difference in noticeable but not huge:
single lockfile: 8 s
separate lockfiles: 13.7 s
I ran make clean-ui first and then pnpm i.
I didn't clear the global store, so dependencies were resolved locally without downloading them from npm.
For comparison, in the same test, it took around 15 s for Yarn 1.x to install the deps, so we would be comparably fast.
There was a problem hiding this comment.
I had an inkling that this is not how the majority of people use pnpm, especially those that work on pnpm itself. I went to pnpm's repo and there's quite a few issues with shared-workspace-lockfile in the title. That's the biggest disadvantage IMHO. We'd be pretty much to square one with how workspaces were treated as experimental in Yarn v1. It does not look like shared-workspace-lockfile is supported on the same level as e.g. pnpm's non-flat node_modules structure.
There was a problem hiding this comment.
Out of all those options, I feel like putting teleport.e deps into the root of package.json might be the best option for now. While not the prettiest and having the potential to corrode the codebase over time, it introduces the least amount of friction and lets us move forward.
Down the road (I hope) we will invert the relationship between OSS and teleport.e. This has been brought up a couple of times and the way I understand it is that instead of teleport.e being a submodule of OSS, OSS would be a submodule of teleport.e.
This would solve the problem with needing a separate package for ent deps. OTOH, we will then have to solve the problem of having two separate lockfiles for each repo.
There was a problem hiding this comment.
That's a good point about the shared-workspace-lockfile=false popularity, when I was looking into that, I couldn't find more information about it, besides that linked documentation.
None of the options that I explored were ideal, I think our case with an "optional" workspace is just uncommon, so the tools don't provide a good option to deal with that.
Inverting the submodule relationship between OSS and e probably would help (this seems to be a more popular scenario).
I'm fine with moving e deps to the root. At least, it won't be worse than what we have today and we can always revisit it in the future.
Thank you @ryanclark and @ravicious, your comments were really helpful.
| - Calling `yarn run ...` will return a clear error that the project uses a | ||
| different package manager. | ||
| 2. Convert the lockfile to a new format (`pnpm import`). | ||
| 3. Update CI, Makefile and package.json scripts. |
There was a problem hiding this comment.
We should also over communicate the change to ensure no one is bit by this
There was a problem hiding this comment.
How about Renovate? We may switch from Dependabot to Renovate in the future..
| Currently (June 2024), we use Yarn 1.x to manage JS dependencies. We should | ||
| upgrade it to Yarn >= 2.x or switch to a different manager. | ||
|
|
||
| ## Way |
There was a problem hiding this comment.
| ## Way | |
| ## Why |
| much when it comes to workspaces, the dependencies are still hoisted. | ||
| * linked: (experimental) install in node_modules/.store, link in place, | ||
| unhoisted. | ||
| Note: workes like pnpm. See an [RFD](https://github.com/npm/rfcs/blob/main/accepted/0042-isolated-mode.md). |
There was a problem hiding this comment.
| Note: workes like pnpm. See an [RFD](https://github.com/npm/rfcs/blob/main/accepted/0042-isolated-mode.md). | |
| Note: works like pnpm. See an [RFD](https://github.com/npm/rfcs/blob/main/accepted/0042-isolated-mode.md). |
|
Rendered version