-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Always include CSS imports from live JS modules #1458
Conversation
I confirmed this PR fixes the issue for me on @sourcegraph's fairly large repo (and does not introduce any other issues as best I can tell). (We use I wonder, however, if a different approach to the fix would be better. This fix leaves the current (I defer to @evanw here, of course. And I'm happy to make a diff for the approach I proposed.) |
@sqs that's a great idea and it might avoid conflicts with any future CSS modules implementation, where CSS files may have non-live parts. I'll see if I can get it working. |
Did some investigation and now I remember why I didn't make the change in So I think the solution might be even earlier in the pipeline, possibly in |
I rebased this PR on the latest main...sqs:esbuild:fix-css-export-star-issue I am not the PR author, so I can't update this PR, but I'm happy to make another PR for ease of merging. @evanw: Would you consider merging this? Or do you have another preferred way of solving this? This would let us switch to esbuild at Sourcegraph, which would be a huge improvement because esbuild is awesome. |
4e6e7f5
to
cb300be
Compare
This is essentially blocked on evanw/esbuild#1458 because: - That PR is needed to make tree-shaking work with esbuild in our app. - We could just use esbuild for dev (where tree-shaking isn't as necessary) and Webpack in prod, but we don't want to use a different builder in dev vs. prod. - Using an esbuild fork is cumbersome, since we'd also need to build the `esbuild` binaries for various platforms.
- DEV_WEB_BUILDER_ESBUILD_FORCE_TREESHAKING: forces tree-shaking. Requires using a fork of esbuild (evanw/esbuild#1458) for now, but merging this will make it easier for more people to experiment with that esbuild fork. - DEV_WEB_BUILDER_UNSAFE_FAST: Setting the env var DEV_WEB_BUILDER_UNSAFE_FAST skips various operations in frontend dev. It's not safe, but if you know what you're doing, go ahead and use it. (CI will catch any issues you forgot about.)
- DEV_WEB_BUILDER_ESBUILD_FORCE_TREESHAKING: forces tree-shaking. Requires using a fork of esbuild (evanw/esbuild#1458) for now, but merging this will make it easier for more people to experiment with that esbuild fork. - DEV_WEB_BUILDER_UNSAFE_FAST: Setting the env var DEV_WEB_BUILDER_UNSAFE_FAST skips various operations in frontend dev. It's not safe, but if you know what you're doing, go ahead and use it. (CI will catch any issues you forgot about.)
This is essentially blocked on evanw/esbuild#1458 because: - That PR is needed to make tree-shaking work with esbuild in our app. - We could just use esbuild for dev (where tree-shaking isn't as necessary) and Webpack in prod, but we don't want to use a different builder in dev vs. prod. - Using an esbuild fork is cumbersome, since we'd also need to build the `esbuild` binaries for various platforms.
Closes #1370.
This change isn't exactly what you described in the linked issue (specifically traversing
export *
statements to find live.css
files), but I think it represents the least surprising behavior when it comes to CSS imported from JS files:With this change, CSS imported from JS will get included in output bundles if the CSS file itself is live and its direct importing JS file is also live. Previously, a dead ancestor would cause an otherwise live CSS file to be excluded.
This new behavior could potentially lead to unused CSS being included in bundles, but we can't determine that statically based only on the import graph. If and when CSS modules are supported, this behavior would remain valid for bare CSS imports, while CSS module imports could be analyzed for DCE based on their synthesized exports.
One thing to note, and a potential deviation from webpack's behavior, is that even with
sideEffects: false
, CSS imports will still be included if their parent JS is included. I believe this is because esbuild (correctly) considers all CSS files as having side effects since CSS modules are not currently supported. So even thoughsideEffects: false
is not technically being respected, the resulting behavior is most likely what the user would be expecting.