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

Unexpected (CSS) import removal with glob pattern in sideEffects array #1370

Closed
jgoz opened this issue Jun 13, 2021 · 5 comments
Closed

Unexpected (CSS) import removal with glob pattern in sideEffects array #1370

jgoz opened this issue Jun 13, 2021 · 5 comments

Comments

@jgoz
Copy link
Contributor

jgoz commented Jun 13, 2021

Repro: https://github.com/jgoz/esbuild-side-effect-glob-issue

Some component libraries are built using global (non-module) CSS where each component imports a .css file as a side-effect import. A component module itself might be otherwise side-effect-free and rightfully subject to DCE, but its CSS import(s) should be included in the CSS bundle if and only if the component is included in the JS bundle.

When using Webpack with css-loader, this behavior can be achieved by specifying "sideEffects": ["*.css"], which essentially indicates that for any module importing a .css file, that import will be preserved since it is side-effect-having.

In esbuild, however, specifying sideEffects per the above may result in those CSS imports being eliminated if the importing JS module has been re-exported through a dependency graph that has been determined to be side-effect-free.

This can be observed in the repro repository where the root exports of the a package are determined to be side-effect-free, since they do not match any pattern in the sideEffects array, which results in a.css being stripped from the CSS output file even though the importing JS is included in the JS output file.

Workarounds:

  • As a package maintainer, add all paths that might import CSS to sideEffects, which is tedious
  • As an esbuild user, set treeShaking: 'ignore-annotations'

This is probably related to #1184 and its fix in e79a747.

I've also pasted the repro as a test case below.

Test case

func TestImportCSSFromJSInPackage(t *testing.T) {
	css_suite.expectBundled(t, bundled{
		files: map[string]string{
			"/Users/user/project/src/entry.js": `
				import { a } from "a";
				import { b } from "b";
				a();
				b();
			`,
			"/Users/user/project/node_modules/a/package.json": `
				{ "sideEffects": ["*.css"] }
			`,
			"/Users/user/project/node_modules/a/index.js": `
				export * from './a';
			`,
			"/Users/user/project/node_modules/a/a.js": `
				import "./a.css";
				export function a() { console.log("a") }
			`,
			"/Users/user/project/node_modules/a/a.css": `
				.a { color: red }
			`,
			"/Users/user/project/node_modules/b/package.json": `
				{}
			`,
			"/Users/user/project/node_modules/b/index.js": `
				export * from './b';
			`,
			"/Users/user/project/node_modules/b/b.js": `
				import "./b.css";
				export function b() { console.log("b") }
			`,
			"/Users/user/project/node_modules/b/b.css": `
				.b { color: blue }
			`,
		},
		entryPaths: []string{"/Users/user/project/src/entry.js"},
		options: config.Options{
			Mode:         config.ModeBundle,
			AbsOutputDir: "/out",
		},
	})
}

Expected snapshot

================================================================================
TestImportCSSFromJSInPackage
---------- /out/entry.js ----------
// Users/user/project/node_modules/a/a.js
function a() {
  console.log("a");
}

// Users/user/project/node_modules/b/b.js
function b() {
  console.log("b");
}

// Users/user/project/src/entry.js
a();
b();

---------- /out/entry.css ----------
/* Users/user/project/node_modules/a/a.css */
.a {
  color: red;
}

/* Users/user/project/node_modules/b/b.css */
.b {
  color: blue;
}

Actual snapshot

================================================================================
TestImportCSSFromJSInPackage
---------- /out/entry.js ----------
// Users/user/project/node_modules/a/a.js
function a() {
  console.log("a");
}

// Users/user/project/node_modules/b/b.js
function b() {
  console.log("b");
}

// Users/user/project/src/entry.js
a();
b();

---------- /out/entry.css ----------
/* Users/user/project/node_modules/b/b.css */
.b {
  color: blue;
}
@evanw
Copy link
Owner

evanw commented Jun 25, 2021

Thanks for the detailed bug report. I can confirm the problem.

This is probably related to #1184 and its fix in e79a747.

I don't think that's the case. It looks like the *.css file is being correctly identified as having side effects internally. But something about the linking process is still excluding it. I think the issue might be with the code that constructs the list of CSS files to bundle together by scanning over the JS files looking for imports. The export * as statements are not marked as live because they aren't included in the final bundle, but they need to be traversed to get to the *.css file. This will need to be fixed.

@hardfist
Copy link
Contributor

@evanw @jgoz is there any workaround to fix this problem before fixing ?

@markdalgleish
Copy link

We've had Remix users reporting this issue when re-exporting components with CSS side-effects (via regular CSS, CSS Modules and Vanilla Extract): remix-run/remix#5270

@markdalgleish
Copy link

markdalgleish commented Jan 26, 2023

I was able to work around this issue by replacing export * from with explicit named re-exports, .e.g.

-export * from "./example";
+export { foo, bar } from "./example";

@evanw evanw closed this as completed in 88e17d8 Feb 9, 2023
sqs added a commit to sourcegraph/sourcegraph that referenced this issue Feb 13, 2023
esbuild is an alternative to Webpack for TypeScript builds in local dev (https://docs.sourcegraph.com/dev/background-information/web/build#esbuild).

The latest release of esbuild fixes an issue (evanw/esbuild#1370) that prevented us from using treeshaking. With esbuild 0.17.7, we can use treeshaking, which means smaller bundle sizes.
sqs added a commit to sourcegraph/sourcegraph that referenced this issue Feb 13, 2023
esbuild is an alternative to Webpack for TypeScript builds in local dev
(https://docs.sourcegraph.com/dev/background-information/web/build#esbuild).

The latest release of esbuild fixes an issue
(evanw/esbuild#1370) that prevented us from
using treeshaking. With esbuild 0.17.7, we can use treeshaking, which
means smaller bundle sizes.




## Test plan

n/a (dev only)

## App preview:

-
[Web](https://sg-web-sqs-upgrade-esbuild-treeshaking.onrender.com/search)

Check out the [client app preview
documentation](https://docs.sourcegraph.com/dev/how-to/client_pr_previews)
to learn more.
@markdalgleish
Copy link

Just a note for anyone following along, even though this issue has been closed due to the fix released in v0.17.7, the fix has actually introduced another issue for those who are relying on CSS tree shaking. The issue can be tracked here: #2933.

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 a pull request may close this issue.

4 participants