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

fix: add styles to sideEffects #408

Merged
merged 3 commits into from
Aug 16, 2021

Conversation

andreafalzetti
Copy link

Description

Changes proposed in this pull request:

  • Make styles/* excluded from webpack tree shaking using sideEffects in package.json

Related issue(s)

When imported the styles should cause side effects, but because the current release (I am using 1.0.0-next.15) has "sideEffects": false, in package.json, webpack tree shakes the import '@asyncapi/react-component/styles/default.css'; at build time.

A workaround for us it's using require instead of import just for this specific file but I rather fix the root issue than mixing require/import.

I have tested locally and works as expected. I am open to different approaches if you thought of one, but according to webpack docs, sideEffects should be used exactly for this situation, let me know your thoughts!

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Welcome to AsyncAPI. Thanks a lot for creating your first pull request. Please check out our contributors guide and the instructions about a basic recommended setup useful for opening a pull request.

Keep in mind there are also other channels you can use to interact with AsyncAPI community. For more details check out this issue.

@magicmatatjahu
Copy link
Member

magicmatatjahu commented Aug 12, 2021

@andreafalzetti Hi! Thank you very much for PR and good explanation why you wanna introduce this changes. To be honest I'm very surprised that webpack tree-shakes the styles when you explicit defined import for them (as I understand). I currently read the docs for webpack and here we have:

Note that any imported file is subject to tree shaking. This means if you use something like css-loader in your project and import a CSS file, it needs to be added to the side effect list so it will not be unintentionally dropped in production mode.

So I guess that css doesn't produce an export and due to this webpack drop it. Good to know fo the future 😅

Also as I understand, the another workaround is making something like that:

import '@asyncapi/react-component/styles/default.css';
import styles from '@asyncapi/react-component/styles/default.css';
console.log(styles);

I also found another possible solution -> https://linguinecode.com/post/reduce-css-file-size-webpack-tree-shaking Do you use the custom webpack or webpack from cli like react-script? If this first, could you test it?

I will accept changes and merge it, but I'm very interesting if you can change the config for tree-shaking of particular module in your webpack.If you use predefined webpack, please let me know and I will merge it immediatelly. And sorry for problems 😅

Also please change the title of issue to the fix: prefix. I know that you use the properly Angular conventions but I don't know if it will fire the patch release in our workflow.

@andreafalzetti andreafalzetti changed the title build(package.json): add styles to sideEffects fix(package.json): add styles to sideEffects Aug 12, 2021
@andreafalzetti
Copy link
Author

andreafalzetti commented Aug 12, 2021

@andreafalzetti Hi! Thank you very much for PR and good explanation why you wanna introduce this changes. To be honest I'm very surprised that webpack tree-shakes the styles when you explicit defined import for them (as I understand). I currently read the docs for webpack and here we have:

Note that any imported file is subject to tree shaking. This means if you use something like css-loader in your project and import a CSS file, it needs to be added to the side effect list so it will not be unintentionally dropped in production mode.

So I guess that css doesn't produce an export and due to this webpack drop it. Good to know fo the future 😅

Also as I understand, the another workaround is making something like that:

import '@asyncapi/react-component/styles/default.css';
import styles from '@asyncapi/react-component/styles/default.css';
console.log(styles);

I also found another possible solution -> linguinecode.com/post/reduce-css-file-size-webpack-tree-shaking Do you use the custom webpack or webpack from cli like react-script? If this first, could you test it?

I will accept changes and merge it, but I'm very interesting if you can change the config for tree-shaking of particular module in your webpack.If you use predefined webpack, please let me know and I will merge it immediatelly. And sorry for problems 😅

Also please change the title of issue to the fix: prefix. I know that you use the properly Angular conventions but I don't know if it will fire the patch release in our workflow.

Thanks for the quick reply! :)

No problem at all, actually thanks a lot for building and publishing this package open source!

First, I have updated the PR title and also commit message using the fix prefix :)

So I guess that css doesn't produce an export and due to this webpack drop it. Good to know fo the future 😅

That's right! After an investigation, I arrived at the same conclusion. The webpack docs that you mentioned seem correct and coherent to the behaviour that I have observed.

The workaround of console.log is clever, I guess, also a bit hacky :D But definitelly a good debugging tip!

We have a custom webpack config, and sideEffects is already set to true :)

Btw - not relevant to the discussion but.. Did you see the new feature from GitHub? Codespaces. I was able to raise this PR without having to clone/install anything on my machine, I did eveything from the browser, including git commits 😍

package.json Outdated
Comment on lines 109 to 112
"sideEffects": [
"./style/default.css",
"./style/default.min.css"
]
Copy link
Member

Choose a reason for hiding this comment

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

@andreafalzetti Thanks for your comment. I heard about that but I didn't use yet, but my friend used and said that its great! Sorry, but I was in Friday off.

Before merge - it should be moved to the https://github.com/asyncapi/asyncapi-react/blob/master/library/package.json place (package.json of component) - here is a package.json where we have shared dependencies across project (we use the lerna for multirepo).

Copy link
Author

@andreafalzetti andreafalzetti Aug 16, 2021

Choose a reason for hiding this comment

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

Makes sense, good catch! Moved. Yon can review this in commit 60117f0

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

1 similar comment
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@magicmatatjahu magicmatatjahu changed the title fix(package.json): add styles to sideEffects fix: add styles to sideEffects Aug 16, 2021
@magicmatatjahu magicmatatjahu merged commit 19f8558 into asyncapi:next Aug 16, 2021
@magicmatatjahu
Copy link
Member

@andreafalzetti New version next.16 should be released very soon. If you will have problems, please ping me here :)

@asyncapi-bot
Copy link
Contributor

🎉 This PR is included in version 1.0.0-next.16 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants