Skip to content

Commit

Permalink
fix #1370: include CSS imports from live JS modules
Browse files Browse the repository at this point in the history
  • Loading branch information
@jgoz
jgoz committed Jul 18, 2021
1 parent 7d630eb commit 4e6e7f5
Show file tree
Hide file tree
Showing 3 changed files with 234 additions and 8 deletions.
145 changes: 145 additions & 0 deletions internal/bundler/bundler_css_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -620,3 +620,148 @@ func TestCSSAndJavaScriptCodeSplittingIssue1064(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",
},
})
}

// 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",
},
})
}
26 changes: 18 additions & 8 deletions internal/bundler/linker.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2701,10 +2701,10 @@ func sanitizeFilePathForVirtualModulePath(path string) string {
// 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, ast.Index32)
var visit func(uint32, ast.Index32, bool)

// Include this file and all files it imports
visit = func(sourceIndex uint32, importerIndex ast.Index32) {
visit = func(sourceIndex uint32, importerIndex ast.Index32, parentIsLive bool) {
if visited[sourceIndex] {
return
}
Expand All @@ -2719,9 +2719,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 @@ -2731,19 +2737,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(), ast.MakeIndex32(sourceIndex))
visit(record.SourceIndex.GetIndex(), ast.MakeIndex32(sourceIndex), 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, ast.Index32{})
visit(entryPoint, ast.Index32{}, true)

return
}
Expand Down
71 changes: 71 additions & 0 deletions internal/bundler/snapshots/snapshots_css.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -239,6 +239,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;
}

================================================================================
TestPackageURLsInCSS
---------- /out/entry.css ----------
Expand Down

0 comments on commit 4e6e7f5

Please sign in to comment.