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.

Sign up for GitHub

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

Jump to bottom

Always include CSS imports from live JS modules #1458

Closed
wants to merge 1 commit into from
Closed

Always include CSS imports from live JS modules #1458

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
145 changes: 145 additions & 0 deletions internal/bundler_tests/bundler_css_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -746,6 +746,54 @@ func TestMetafileCSSBundleTwoToOne(t *testing.T) {
})
}

// CSS from JS via re-export where '.css' files are marked as sideEffects
func TestImportCSSFromJSInPackageWithArraySideEffects(t *testing.T) {
css_suite.expectBundled(t, bundled{
files: map[string]string{
"/Users/user/project/src/entry.js": `
import { a } from "a";
a();
`,
// "a.css" should get included because "a.js" is imported,
// but "aa.css" should not get included
"/Users/user/project/node_modules/a/package.json": `
{ "sideEffects": ["*.css"] }
`,
"/Users/user/project/node_modules/a/index.js": `
export * from './a';
export * from './aa';
`,
"/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": `
@import "z/z.css";
.a { color: red }
`,
"/Users/user/project/node_modules/a/aa.js": `
import "./aa.css";
export function aa() { console.log("aa") }
`,
"/Users/user/project/node_modules/a/aa.css": `
@import "z/z.css";
.aa { color: darkred }
`,
"/Users/user/project/node_modules/z/package.json": `
{}
`,
"/Users/user/project/node_modules/z/z.css": `
.z { color: black }
`,
},
entryPaths: []string{"/Users/user/project/src/entry.js"},
options: config.Options{
Mode: config.ModeBundle,
AbsOutputDir: "/out",
},
})
}

func TestDeduplicateRules(t *testing.T) {
// These are done as bundler tests instead of parser tests because rule
// deduplication now happens during linking (so that it has effects across files)
Expand Down Expand Up @@ -791,3 +839,100 @@ func TestDeduplicateRules(t *testing.T) {
expectedScanLog: "no0.css: WARNING: CSS nesting syntax cannot be used outside of a style rule\n",
})
}

// CSS from JS via re-export where sideEffects is not specified
func TestImportCSSFromJSInPackageWithEmptySideEffects(t *testing.T) {
css_suite.expectBundled(t, bundled{
files: map[string]string{
"/Users/user/project/src/entry.js": `
import { b } from "b";
b();
`,
// Both "b.css" and "bb.css" should get included because
// not specifying "sideEffects" will conservatively mark
// any side-effect import for inclusion
"/Users/user/project/node_modules/b/package.json": `
{}
`,
"/Users/user/project/node_modules/b/index.js": `
export * from './b';
export * from './bb';
`,
"/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": `
@import "z/z.css";
.b { color: blue }
`,
"/Users/user/project/node_modules/b/bb.js": `
import "./bb.css";
export function bb() { console.log("bb") }
`,
"/Users/user/project/node_modules/b/bb.css": `
@import "z/z.css";
.bb { color: skyblue }
`,
"/Users/user/project/node_modules/z/package.json": `
{}
`,
"/Users/user/project/node_modules/z/z.css": `
.z { color: black }
`,
},
entryPaths: []string{"/Users/user/project/src/entry.js"},
options: config.Options{
Mode: config.ModeBundle,
AbsOutputDir: "/out",
},
})
}

// CSS from JS via re-export where sideEffects is false
func TestImportCSSFromJSInPackageWithFalseSideEffects(t *testing.T) {
css_suite.expectBundled(t, bundled{
files: map[string]string{
"/Users/user/project/src/entry.js": `
import { c } from "c";
c();
`,
// Even though we specify "sideEffects": false, "c.css" should get
// included because its importing module "c.js" is included
"/Users/user/project/node_modules/c/package.json": `
{ "sideEffects": false }
`,
"/Users/user/project/node_modules/c/index.js": `
export * from './c';
export * from './cc';
`,
"/Users/user/project/node_modules/c/c.js": `
import "./c.css";
export function c() { console.log("c") }
`,
"/Users/user/project/node_modules/c/c.css": `
@import "z/z.css";
.c { color: yellow }
`,
"/Users/user/project/node_modules/c/cc.js": `
import "./cc.css";
export function cc() { console.log("cc") }
`,
"/Users/user/project/node_modules/c/cc.css": `
@import "z/z.css";
.cc { color: goldenrod }
`,
"/Users/user/project/node_modules/z/package.json": `
{}
`,
"/Users/user/project/node_modules/z/z.css": `
.z { color: black }
`,
},
entryPaths: []string{"/Users/user/project/src/entry.js"},
options: config.Options{
Mode: config.ModeBundle,
AbsOutputDir: "/out",
},
})
}
71 changes: 71 additions & 0 deletions internal/bundler_tests/snapshots/snapshots_css.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -398,6 +398,77 @@ console.log("b");
color: blue;
}

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

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

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

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

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

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

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

/* Users/user/project/node_modules/z/z.css */
.z {
color: black;
}

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

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

// Users/user/project/src/entry.js
c();

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

/* Users/user/project/node_modules/c/c.css */
.c {
color: yellow;
}

================================================================================
TestMetafileCSSBundleTwoToOne
---------- /out/js/UOATE6K4.js ----------
Expand Down
26 changes: 18 additions & 8 deletions internal/linker/linker.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2901,10 +2901,10 @@ func (c *linkerContext) markPartLiveForTreeShaking(sourceIndex uint32, partIndex
// In this case we just pick an arbitrary but consistent order.
func (c *linkerContext) findImportedCSSFilesInJSOrder(entryPoint uint32) (order []uint32) {
visited := make(map[uint32]bool)
var visit func(uint32)
var visit func(uint32, bool)

// Include this file and all files it imports
visit = func(sourceIndex uint32) {
visit = func(sourceIndex uint32, parentIsLive bool) {
if visited[sourceIndex] {
return
}
Expand All @@ -2919,9 +2919,15 @@ func (c *linkerContext) findImportedCSSFilesInJSOrder(entryPoint uint32) (order
// imports, could end up being activated by the bundle and needs its
// CSS to be included. This may change if/when code splitting is
// supported for CSS.
if !part.IsLive {
continue
}
//
// Note that for a CSS file imported by a JS file, liveness of the CSS file
// is actually determined by the JS file. CSS imports (except for pure
// CSS modules, which are not currently supported by esbuild) are always
// side-effect imports, so if the importing module is live, it's safe to
// assume that the CSS file it imports must also be live. Strictly speaking,
// the imported CSS might not actually be used, but there is no way to
// statically determine that.
importIsLive := part.IsLive

// Traverse any files imported by this part. Note that CommonJS calls
// to "require()" count as imports too, sort of as if the part has an
Expand All @@ -2931,19 +2937,23 @@ func (c *linkerContext) findImportedCSSFilesInJSOrder(entryPoint uint32) (order
// this is the only way to do it.
for _, importRecordIndex := range part.ImportRecordIndices {
if record := &repr.AST.ImportRecords[importRecordIndex]; record.SourceIndex.IsValid() {
visit(record.SourceIndex.GetIndex())
visit(record.SourceIndex.GetIndex(), importIsLive)
}
}
}

// Iterate over the associated CSS imports in postorder
if repr.CSSSourceIndex.IsValid() {
order = append(order, repr.CSSSourceIndex.GetIndex())
cssIndex := repr.CSSSourceIndex.GetIndex()
cssFile := c.graph.Files[cssIndex]
if parentIsLive && cssFile.IsLive {
order = append(order, cssIndex)
}
}
}

// Include all files reachable from the entry point
visit(entryPoint)
visit(entryPoint, true)

return
}
Expand Down