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

Added styles.css to sideEffects #1738

Merged
merged 2 commits into from
Dec 14, 2023
Merged

Added styles.css to sideEffects #1738

merged 2 commits into from
Dec 14, 2023

Conversation

Woyken
Copy link
Contributor

@Woyken Woyken commented Dec 14, 2023

This fix is intended for webpack users.
When using ESM, webpack will apply it's treeshaking too harshly sometimes.
This applies to imported css files too.
Possible webpack bug: webpack/webpack#7094

Fixes #1737

Changes

Added styles.css to sideEffects in package.json

Testing

"N/A"

Docs

"N/A"

for webpack to not treeshake out the css file
@Woyken Woyken requested review from a team as code owners December 14, 2023 09:10
@Woyken Woyken requested review from mayank99 and siddhantrawal and removed request for a team December 14, 2023 09:10
@CLAassistant
Copy link

CLAassistant commented Dec 14, 2023

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@rohan-kadkol rohan-kadkol left a comment

Choose a reason for hiding this comment

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

After reading the webpack docs about tree-shaking

Screenshot 2023-12-14 at 9 22 20 AM

... and this StackOverflow answer, I believe the change in this PR is alright. But let's wait for another review.

Copy link
Contributor

@mayank99 mayank99 left a comment

Choose a reason for hiding this comment

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

I've always found sideEffects to be confusing. Using this field in the past, we've had problems with some bundlers including all .css files even if their components are not actually used. But now that we only have a single stylesheet, that should be fine I guess.

I also thought "sideEffects": false would be the correct move, since we do not import it within our components, but apparently not.

My only remaining concern is regarding packages that depend on itwinui-react. If they use a bundler, it might decide to include styles.css even if it's not imported. But packages really should not be using bundlers in the first place, so maybe it's not a big concern.

Lets go with this and see if anybody complains. 👍

@mayank99 mayank99 merged commit 0695bb3 into iTwin:main Dec 14, 2023
9 of 10 checks passed
@imodeljs-admin imodeljs-admin mentioned this pull request Dec 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing styles.css in package.json sideEffects
4 participants