feat: added --install-strategy=linked#6078
Conversation
Co-authored-by: Vincent Bailly <vibailly@microsoft.com>
fritzy
force-pushed
the
fritzy/linked-install3
branch
from
3caaf36 to
2f8a505
Compare
|
no statistically significant performance changes detected timing results
|
|
Did it run using How can we run these perf tests using linked install? Shouldn't we see performnace improvements here?
|
|
I'm not sure how automated performance tests used to detect regressions could be expected to know about a new flag in a PR and use it? That's not what that test is for. |
|
agree for regression this is good and doing the right thing. for this specific change we would love to see benchmark and perf suites results with new flag. How can i trigger that? |
|
@saquibkhan I have a benchmarks branch that will address this. |
fritzy
force-pushed
the
fritzy/linked-install3
branch
from
31aba55 to
ea39f75
Compare
|
|
Co-authored-by: Vincent Bailly <vibailly@microsoft.com> Implements the RFC https://github.com/npm/rfcs/blob/main/accepted/0042-isolated-mode.md Packages are installed in node_modules/.store flat, and linked into the node_modules tree in depth, rather than hoisted.
|
I hope this works with workspaces... |
## 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.
|
Hi! Could you more deeply explain how works Or there will be shared dependency cache, with links from packages |
9 participants
Co-authored-by: Vincent Bailly vibailly@microsoft.com
Implements the RFC https://github.com/npm/rfcs/blob/main/accepted/0042-isolated-mode.md
Packages are installed in
node_modules/.storeflat, and linked into thenode_modulestree in depth, rather than hoisted.