Implements the pnpm linker #3338
Conversation
| // Copy the package source into the <root>/n_m/.store/<hash> directory, so | ||
| // that we can then create symbolic links to it later. | ||
| await xfs.copyPromise(pkgPath, fetchResult.prefixPath, { | ||
| baseFs: fetchResult.packageFs, |
There was a problem hiding this comment.
Wont the core release this before the copy completes?
There was a problem hiding this comment.
Of course 🤦♀️ completely forgot that. I've implemented a holdFetchResult that instructs the core not to reclaim the fetchResult until the given promise has resolved. Does that sound more elegant than the previous design?
There was a problem hiding this comment.
It's better but I had more of a ref counter / shared_ptr like system in mind so the fetchResult itself contains the logic and not the core
There was a problem hiding this comment.
This would likely require a heavier refactoring than what I feel comfortable doing for this feature 🤔
|
Really excited to try this out, thanks @arcanis! |
|
@arcanis exciting subject... I'm currently trying out the pnpm linker with yarn 3.1.0-rc.8. The P/R is here: belgattitude/nextjs-monorepo-example#555 It's a workspace enabled project (monorepo), and took the time to remove any possible warning at install (based on nmLinker: node_modules). Then changed the nmLinker to pnpm. At this stage I spotted few bugs that prevent installation... But I'm wondering if rather than opening few issues, we could open a discussion about this like title: [Feedback thread] PNPM style node module linker PS: the idea is based on vercel/next.js#27876 which I found out super useful. That would give some visibility to the feature, and eventually reduce your maintenance work as community would be able to help. That said I'm open to create tickets too but I prefer to first ask your opinion on this. |
|
I think I'd tend to prefer regular independent issues, since then we can have Sherlock to get repros + are able to add the "good first issue" / "help wanted" tags if relevant |
|
Looks good to me. PS: to not create too much noise, I'll try to get to the berry code first. I saw few limitations that I wasn't aware like Will try to log which packages causes this (prisma...) once I'm up and running. Thanks for the feature |
## Summary <!-- Ideally, there is an attached GitHub issue that will describe the "why". If relevant, use this section to call out any additional information you'd like to _highlight_ to the reviewer. --> _This is part of a [series](#4813) of PRs being spun off from [my WIP branch](lewisl9029#2) to get the Highlight web app ready for [Reflame](https://reflame.app/). Hopefully this makes things a bit easier to review, test, and merge. 🙂_ There were several places in the frontend codebase where we were importing from `react-router`, a transitive dependency, instead of `react-router-dom`, a direct dependency. This is often referred to as a phantom dependency, and can cause a number of issues, both in theory and practice: - We have a rule against importing `useParams` from `react-router-dom`, but that does nothing to protect against importing `useParams` from `react-router`. In fact, this was something I [had to fix](38a02f4#diff-5fd6e69a538cbc878adc5b71eb5d9a6d68cd53d6588689284f4ecd9506343681L49) as part of this PR. - Since the versions of transitive deps are not specified explicitly, they can easily change under our feet without us noticing and potentially cause us to import a different version than we were expecting, or can even inexplicably disappear altogether when seemingly unrelated deps change. The potential for spooky actions at a distance is greatly exacerbated in a large monorepo like we have here. The Rush.js folks did a great [writeup](https://rushjs.io/pages/advanced/phantom_deps/) on the perils of phathom dependencies, so I won't rehash all the details. - It's incompatible with stricter package management schemes like pnpm (and the one used by [Reflame](https://reflame.app/) itself, which was admittedly my primary motivation for this PR 😅) that don't expose non-direct dependencies to the resolution algorithm to begin with, specifically to combat the phantom dependency problem. All I did to fix this was to search & replace all references of `'react-router'` to `'react-router-dom'`. And to prevent this specific issue from happening again, I added `react-router` to our existing list of restricted imports in eslint. For a more thorough defense against phantom deps, we'll need to switch to something like pnpm, npm's new [`linked` install strategy](npm/cli#6078), or possibly yarn's [pnpm nodeLinker option](yarnpkg/berry#3338) for a less disruptive migration (though I have no idea if it does what I think it does). ## How did you test this change? <!-- Frontend - Leave a screencast or a screenshot to visually describe the changes. --> I ran `yarn turbo run dev --filter frontend...` locally and poked around the app. ## Are there any deployment considerations? <!-- Backend - Do we need to consider migrations or backfilling data? --> None that I can think of.
What's the problem this PR addresses?
Given that Yarn has all the infrastructure needed to easily swap from one linker to another, I was a little curious how much work it'd be to support a symlink-based install approach, similar to the pnpm install strategy. I made a quick prototype a few months ago, which was mostly working except for a few little details, and decided to clean it up a bit.
Closes #1845
How did you fix it?
This PR adds a
plugin-pnpmpackage that exposes thePnpmLinker. It doesn't exactly follow the pnpm installs (the folder names are different, etc), but overall it should be more or less the same thing. In order to enable it, the relevant setting must be enabled in your.yarnrc.yml:Note that
pnpstill remains the default install strategy, as node_modules-based approach (both hoisted and symlink-based) are incapable of ensuring perfect boundaries and semantics (for instance, workspaces' peer dependencies cannot be followed).Checklist