-
Notifications
You must be signed in to change notification settings - Fork 1.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix incorrectly hoisted JS dependencies #33356
Comments
As a stop gap, we could add |
Sounds good to me. |
corepack can automatically install the correct version of a specific package manager. This seems like a super good feature, as people wouldn't have to manually manage the version of whatever manager that we pick. |
Yarn v1 has a bug related to hoisting dependencies when using workspaces. Instead of hoisting dependencies as defined in package.json of our workspaces, the hoisting algorithm seems to be impacted by transitive dependencies as well.
For example, if
web/packages/build/package.json
depends on @babel/core ^7.23.2 and @storybook/builder-webpack5 ^6.5.16 which in turn depends on @babel/core ^7.12.10, Yarn will hoist the @babel/core version resolved from that storybook dependency rather than the direct @babel/core dep in package.json. What's more, it doesn't even seem to dedupe those deps correctly despite overlapping version ranges for @babel/core.Solutions
The Yarn team has stated that this won't be fixed in v1, so if we want to address this, we have to either upgrade to v2 or move to another package manager.
From my own attemtps, upgrading to v2 doesn't make me feel optimistic as the official migration guide does not account for such a simple thing as informing you that you should empty all node_modules dirs in the project (or it could just do that for you). Ryan also had mixed feelings from his recent attempt to upgrade one of his projects to v2.
Alas, when it comes to other package managers pnpm doesn't seem to work properly with electron-builder due to how electron-builder approaches bundling deps. bun has the issue with a binary lockfile format, meaning that on every conflict we'd have to throw away the lockfile and start fresh – this is not something you want to do on a git conflict as it unceremoniously updates all packages. See the discussion in bun's repo for more details.
npm would be another candidate to investigate. IIRC, npm has a rather unorthodox approach to caching which involves some additional setup if you want to, for example, cache your Docker layers properly.
npm clean-install
is the only method to install packages while respecting the lockfile. But it has the quirk where it'll first remove node_modules completely. So you not only have to cache node_modules but also npm's cache folder. Also, while it does seem to support workspaces, it's unknown how well it does so.See also Grzegorz's report from trying to update Yarn to v2 and changing how we approach hoisting deps within workspaces.
Alternatives
An alternative would be to dedupe overlaping version ranges by running yarn-deduplicate manually and then following up with another invocation of
yarn install
to clean up any orphaned packages. I remembered that @jentfoo used yarn-deduplicate a while back. Calling it without any args introduces a big change though. Fortunately, it also supports flags to minimize the amount of changes.In #33355, I updated and deduplicated babel deps by running
npx yarn-deduplicate yarn.lock --scopes @babel
. The tool also supports the--packages
flag for individual packages. This is going to work as long as both Storybook and our workspaces have overlapping version ranges. I don't know how Yarn is going to behave if those ranges don't overlap.The text was updated successfully, but these errors were encountered: