-
Notifications
You must be signed in to change notification settings - Fork 4
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(build): add css as having side effects #1481
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tyty! could you add to the test plan how you verified the fix with alpha publishes?
size-limit report 📦
|
Codecov Report
@@ Coverage Diff @@
## next #1481 +/- ##
==========================================
- Coverage 91.54% 91.49% -0.05%
==========================================
Files 279 279
Lines 4234 4234
Branches 755 755
==========================================
- Hits 3876 3874 -2
- Misses 356 358 +2
Partials 2 2
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Follow-up to #1473 |
"sideEffects": false, | ||
"sideEffects": [ | ||
"lib/tokens/css/variables.css", | ||
"lib/tokens/fonts.css" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's happening on the Traject side without this?
When writing #1473, I didn't think this was necessary because Traject should be importing these explicitly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fonts and CSS custom properties are being lost https://www.chromatic.com/build?appId=614b969675171d003ac6f98a&number=15997
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dang, thanks for digging into this. Looks like you were totally right @anniehu4.
There's still something I don't understand here about how Webpack handles tree shaking 😓
Seems like it is an intended thing, though: webpack/webpack#6741
Yup, done |
Summary:
Test Plan:
@chanzuckerberg/eds: 8.1.0-alpha.4
in LP, where8.1.0-alpha.4
is the latest alpha publish off nexttraject/node_modules/@chanzuckerberg/eds/package.json
changesideEffects: false
tonpm run storybook
and see that storybook is unbroken8.1.0-alpha.5
which is alpha publish off of this branchlib/tokens/fonts.css
insideEffects
: https://www.chromatic.com/build?appId=614b969675171d003ac6f98a&number=16064