Skip to content
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

[Storybook]: Deduplicate babel types to fix build #35659

Merged
merged 2 commits into from Nov 14, 2023
Merged

Conversation

uladzimirdev
Copy link
Contributor

@uladzimirdev uladzimirdev commented Nov 13, 2023

Description

This problem appeared after #35602 where I suggested to upgrade all babel packages

old style package managers like npm or yarn@1 have a well known problem called "doppelgangers", more info.

tl;dr
packageA has a dep of packageD@^1.1.1
packageB has a dep of packageD@^1.1.2
latest version of packageD is 1.1.5

so even if logically we need to install the only version of packageD 1.1.5, which will satisfy every package, npm and yarn@1 do not do it by design (and can't change it as it will break backward compatibility)

In our case there are plenty of installed babel packages: @babel/core, @babel/types etc,
so even if storybook has it's own listed @babel/core and @babel/types (node_modules/@storybook/.../node_modules/@babel/core), sometimes node or webpack doesn't resolve the correct version of the package (e.g. version from node_modules/@babel/core) is found first, which can lead to the broken build:
in our case @babel/types of old version didn't contains some exported API which was used in storybook.

Those issues are pretty hard to track and debug, there are several issues on storybook (like this) about the same and since with yarn@1 and npm even order of adding deps matters for building lockfile, the result is not determenistic.

How did I get the fix

I played with storybook config and got not cryptic error message which led me to the babel issue and the point was: try to avoid duplications in the node_modules

there is a special package for yarn: https://github.com/scinos/yarn-deduplicate

npx yarn-deduplicate --packages @babel/types

^^ updates the lockfile with deduplicated deps. it's almost the same as using resolutions, but more explicit.

Side effect: shrink of node_modules

before:
image

after:
Screenshot 2023-11-14 at 16 23 46

How to verify

check this build https://github.com/metabase/metabase/actions/runs/6852603043?pr=35659

or locally:

rm -rf node_modules && yarn && yarn build-storybook

Copy link

deploysentinel bot commented Nov 13, 2023

No failed tests 🎉

@iethree
Copy link
Contributor

iethree commented Nov 13, 2023

why did yarn let this happen?

@@ -14,6 +14,7 @@ module.exports = {
"@storybook/addon-links",
"@storybook/addon-a11y",
],
babel: () => {},
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

storybook combines our config and their config which leads to the problem of resolving packages.
with such line we use only our babel.config.json without storybook magic

to-fast-properties "^2.0.0"

"@babel/types@^7.22.19", "@babel/types@^7.22.5", "@babel/types@^7.23.0", "@babel/types@^7.23.3":
"@babel/types@^7.0.0", "@babel/types@^7.10.4", "@babel/types@^7.10.5", "@babel/types@^7.11.0", "@babel/types@^7.12.1", "@babel/types@^7.12.11", "@babel/types@^7.12.5", "@babel/types@^7.12.6", "@babel/types@^7.12.7", "@babel/types@^7.14.5", "@babel/types@^7.14.8", "@babel/types@^7.16.0", "@babel/types@^7.18.10", "@babel/types@^7.18.6", "@babel/types@^7.18.9", "@babel/types@^7.19.0", "@babel/types@^7.19.3", "@babel/types@^7.2.0", "@babel/types@^7.20.0", "@babel/types@^7.20.2", "@babel/types@^7.20.5", "@babel/types@^7.20.7", "@babel/types@^7.21.0", "@babel/types@^7.21.3", "@babel/types@^7.21.4", "@babel/types@^7.21.5", "@babel/types@^7.22.0", "@babel/types@^7.22.15", "@babel/types@^7.22.19", "@babel/types@^7.22.3", "@babel/types@^7.22.4", "@babel/types@^7.22.5", "@babel/types@^7.23.0", "@babel/types@^7.23.3", "@babel/types@^7.3.0", "@babel/types@^7.3.3", "@babel/types@^7.4.4", "@babel/types@^7.7.0", "@babel/types@^7.8.3":
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@babel/types should be backward compatible, so newer version is older + some fixes

@uladzimirdev
Copy link
Contributor Author

why did yarn let this happen?

@iethree because yarn does it by design, please check the updated PR description

@uladzimirdev uladzimirdev self-assigned this Nov 14, 2023
@uladzimirdev uladzimirdev added the no-backport Do not backport this PR to any branch label Nov 14, 2023
@npretto
Copy link
Member

npretto commented Nov 14, 2023

Considering that yarn 1.x is in maintainance mode (and will therefore probably never get dedup built-in)
image

Should we also start talking about a migration plan?
A quick google search seems to indicate that npm is still slower than yarn 1.x, but pnpm is faster than both (and has been out for a while so it should be mature enough)

@iethree
Copy link
Contributor

iethree commented Nov 14, 2023

❤️ 🚀 This was the most educational PR description I've read in a long time.

@uladzimirdev uladzimirdev merged commit a7e16c4 into master Nov 14, 2023
108 of 112 checks passed
@uladzimirdev uladzimirdev deleted the fix-babel-deps branch November 14, 2023 18:45
Copy link

@uladzimirdev Did you forget to add a milestone to the issue for this PR? When and where should I add a milestone?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chromatic no-backport Do not backport this PR to any branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants