diff --git a/CHANGELOG.md b/CHANGELOG.md index 6b141e34702..ced565a47db 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,78 @@ The full Unix-y details: Unix permissions use three-digit octal notation where the three digits mean "user, group, other" in that order. Within a digit, 4 means "read" and 2 means "write" and 1 means "execute". So 6 == 4 + 2 == read + write. Previously esbuild uses 0644 permissions (the leading 0 means octal notation) but the permissions for `fs.writeFileSync` defaults to 0666, so esbuild will now use 0666 permissions. This does not necessarily mean that the files esbuild generates will end up having 0666 permissions, however, as there is another Unix feature called "umask" where the operating system masks out some of these bits. If your umask is set to 0022 then the generated files will have 0644 permissions, and if your umask is set to 0002 then the generated files will have 0664 permissions. +* Fix a subtle CSS ordering issue with `@import` and `@layer` + + With this release, esbuild may now introduce additional `@layer` rules when bundling CSS to better preserve the layer ordering of the input code. Here's an example of an edge case where this matters: + + ```css + /* entry.css */ + @import "a.css"; + @import "b.css"; + @import "a.css"; + ``` + + ```css + /* a.css */ + @layer a { + body { + background: red; + } + } + ``` + + ```css + /* b.css */ + @layer b { + body { + background: green; + } + } + ``` + + This CSS should set the body background to `green`, which is what happens in the browser. Previously esbuild generated the following output which incorrectly sets the body background to `red`: + + ```css + /* b.css */ + @layer b { + body { + background: green; + } + } + + /* a.css */ + @layer a { + body { + background: red; + } + } + ``` + + This difference in behavior is because the browser evaluates `a.css` + `b.css` + `a.css` (in CSS, each `@import` is replaced with a copy of the imported file) while esbuild was only writing out `b.css` + `a.css`. The first copy of `a.css` wasn't being written out by esbuild for two reasons: 1) bundlers care about code size and try to avoid emitting duplicate CSS and 2) when there are multiple copies of a CSS file, normally only the _last_ copy matters since the last declaration with equal specificity wins in CSS. + + However, `@layer` was recently added to CSS and for `@layer` the _first_ copy matters because layers are ordered using their first location in source code order. This introduction of `@layer` means esbuild needs to change its bundling algorithm. An easy solution would be for esbuild to write out `a.css` twice, but that would be inefficient. So what I'm going to try to have esbuild do with this release is to write out an abbreviated form of the first copy of a CSS file that only includes the `@layer` information, and then still only write out the full CSS file once for the last copy. So esbuild's output for this edge case now looks like this: + + ```css + /* a.css */ + @layer a; + + /* b.css */ + @layer b { + body { + background: green; + } + } + + /* a.css */ + @layer a { + body { + background: red; + } + } + ``` + + The behavior of the bundled CSS now matches the behavior of the unbundled CSS. You may be wondering why esbuild doesn't just write out `a.css` first followed by `b.css`. That would work in this case but it doesn't work in general because for any rules outside of a `@layer` rule, the last copy shold still win instead of the first copy. + ## 0.19.0 **This release deliberately contains backwards-incompatible changes.** To avoid automatically picking up releases like this, you should either be pinning the exact version of `esbuild` in your `package.json` file (recommended) or be using a version range syntax that only accepts patch upgrades such as `^0.18.0` or `~0.18.0`. See npm's documentation about [semver](https://docs.npmjs.com/cli/v6/using-npm/semver/) for more information. diff --git a/internal/bundler_tests/bundler_css_test.go b/internal/bundler_tests/bundler_css_test.go index f9d8c24b697..203d92ed78f 100644 --- a/internal/bundler_tests/bundler_css_test.go +++ b/internal/bundler_tests/bundler_css_test.go @@ -1771,6 +1771,202 @@ url-format/003/relative-url/style.css: NOTE: The unbalanced "(" is here: }) } +func TestCSSAtImportConditionsAtLayerBundle(t *testing.T) { + css_suite.expectBundled(t, bundled{ + files: map[string]string{ + "/case1.css": ` + @import url(case1-foo.css) layer(first.one); + @import url(case1-foo.css) layer(last.one); + @import url(case1-foo.css) layer(first.one); + `, + "/case1-foo.css": `body { color: red }`, + + "/case2.css": ` + @import url(case2-foo.css); + @import url(case2-bar.css); + @import url(case2-foo.css); + `, + "/case2-foo.css": `@layer first.one { body { color: red } }`, + "/case2-bar.css": `@layer last.one { body { color: green } }`, + + "/case3.css": ` + @import url(case3-foo.css); + @import url(case3-bar.css); + @import url(case3-foo.css); + `, + "/case3-foo.css": `@layer { body { color: red } }`, + "/case3-bar.css": `@layer only.one { body { color: green } }`, + + "/case4.css": ` + @import url(case4-foo.css) layer(first); + @import url(case4-foo.css) layer(last); + @import url(case4-foo.css) layer(first); + `, + "/case4-foo.css": `@layer one { @layer two, three.four; body { color: red } }`, + + "/case5.css": ` + @import url(case5-foo.css) layer; + @import url(case5-foo.css) layer(middle); + @import url(case5-foo.css) layer; + `, + "/case5-foo.css": `@layer one { @layer two, three.four; body { color: red } }`, + + "/case6.css": ` + @import url(case6-foo.css) layer(first); + @import url(case6-foo.css) layer(last); + @import url(case6-foo.css) layer(first); + `, + "/case6-foo.css": `@layer { @layer two, three.four; body { color: red } }`, + }, + entryPaths: []string{ + "/case1.css", + "/case2.css", + "/case3.css", + "/case4.css", + "/case5.css", + "/case6.css", + }, + options: config.Options{ + Mode: config.ModeBundle, + AbsOutputDir: "/out", + }, + }) +} + +func TestCSSAtImportConditionsAtLayerBundleAlternatingLayerInFile(t *testing.T) { + css_suite.expectBundled(t, bundled{ + files: map[string]string{ + "/a.css": `@layer first { body { color: red } }`, + "/b.css": `@layer last { body { color: green } }`, + + "/case1.css": ` + @import url(a.css); + @import url(a.css); + `, + + "/case2.css": ` + @import url(a.css); + @import url(b.css); + @import url(a.css); + `, + + "/case3.css": ` + @import url(a.css); + @import url(b.css); + @import url(a.css); + @import url(b.css); + `, + + "/case4.css": ` + @import url(a.css); + @import url(b.css); + @import url(a.css); + @import url(b.css); + @import url(a.css); + `, + + "/case5.css": ` + @import url(a.css); + @import url(b.css); + @import url(a.css); + @import url(b.css); + @import url(a.css); + @import url(b.css); + `, + + // Note: There was a bug that only showed up in this case. We need at least this many cases. + "/case6.css": ` + @import url(a.css); + @import url(b.css); + @import url(a.css); + @import url(b.css); + @import url(a.css); + @import url(b.css); + @import url(a.css); + `, + }, + entryPaths: []string{ + "/case1.css", + "/case2.css", + "/case3.css", + "/case4.css", + "/case5.css", + "/case6.css", + }, + options: config.Options{ + Mode: config.ModeBundle, + AbsOutputDir: "/out", + }, + }) +} + +func TestCSSAtImportConditionsAtLayerBundleAlternatingLayerOnImport(t *testing.T) { + css_suite.expectBundled(t, bundled{ + files: map[string]string{ + "/a.css": `body { color: red }`, + "/b.css": `body { color: green }`, + + "/case1.css": ` + @import url(a.css) layer(first); + @import url(a.css) layer(first); + `, + + "/case2.css": ` + @import url(a.css) layer(first); + @import url(b.css) layer(last); + @import url(a.css) layer(first); + `, + + "/case3.css": ` + @import url(a.css) layer(first); + @import url(b.css) layer(last); + @import url(a.css) layer(first); + @import url(b.css) layer(last); + `, + + "/case4.css": ` + @import url(a.css) layer(first); + @import url(b.css) layer(last); + @import url(a.css) layer(first); + @import url(b.css) layer(last); + @import url(a.css) layer(first); + `, + + "/case5.css": ` + @import url(a.css) layer(first); + @import url(b.css) layer(last); + @import url(a.css) layer(first); + @import url(b.css) layer(last); + @import url(a.css) layer(first); + @import url(b.css) layer(last); + `, + + // Note: There was a bug that only showed up in this case. We need at least this many cases. + "/case6.css": ` + @import url(a.css) layer(first); + @import url(b.css) layer(last); + @import url(a.css) layer(first); + @import url(b.css) layer(last); + @import url(a.css) layer(first); + @import url(b.css) layer(last); + @import url(a.css) layer(first); + `, + }, + entryPaths: []string{ + "/case1.css", + "/case2.css", + "/case3.css", + "/case4.css", + "/case5.css", + "/case6.css", + }, + options: config.Options{ + Mode: config.ModeBundle, + AbsOutputDir: "/out", + }, + }) +} + // This test mainly just makes sure that this scenario doesn't crash func TestCSSAndJavaScriptCodeSplittingIssue1064(t *testing.T) { css_suite.expectBundled(t, bundled{ diff --git a/internal/bundler_tests/snapshots/snapshots_css.txt b/internal/bundler_tests/snapshots/snapshots_css.txt index 55b02d1534f..4798b5cdad2 100644 --- a/internal/bundler_tests/snapshots/snapshots_css.txt +++ b/internal/bundler_tests/snapshots/snapshots_css.txt @@ -86,6 +86,378 @@ TestCSSAtImport color: red; } +================================================================================ +TestCSSAtImportConditionsAtLayerBundle +---------- /out/case1.css ---------- +/* case1-foo.css */ +@layer first.one; + +/* case1-foo.css */ +@layer last.one { + body { + color: red; + } +} + +/* case1-foo.css */ +@layer first.one { + body { + color: red; + } +} + +/* case1.css */ + +---------- /out/case2.css ---------- +/* case2-foo.css */ +@layer first.one; + +/* case2-bar.css */ +@layer last.one { + body { + color: green; + } +} + +/* case2-foo.css */ +@layer first.one { + body { + color: red; + } +} + +/* case2.css */ + +---------- /out/case3.css ---------- +/* case3-bar.css */ +@layer only.one { + body { + color: green; + } +} + +/* case3-foo.css */ +@layer { + body { + color: red; + } +} + +/* case3.css */ + +---------- /out/case4.css ---------- +/* case4-foo.css */ +@layer first { + @layer one, one.two, one.three.four; +} + +/* case4-foo.css */ +@layer last { + @layer one { + @layer two, three.four; + body { + color: red; + } + } +} + +/* case4-foo.css */ +@layer first { + @layer one { + @layer two, three.four; + body { + color: red; + } + } +} + +/* case4.css */ + +---------- /out/case5.css ---------- +/* case5-foo.css */ +@layer middle { + @layer one { + @layer two, three.four; + body { + color: red; + } + } +} + +/* case5-foo.css */ +@layer { + @layer one { + @layer two, three.four; + body { + color: red; + } + } +} + +/* case5.css */ + +---------- /out/case6.css ---------- +/* case6-foo.css */ +@layer first; + +/* case6-foo.css */ +@layer last { + @layer { + @layer two, three.four; + body { + color: red; + } + } +} + +/* case6-foo.css */ +@layer first { + @layer { + @layer two, three.four; + body { + color: red; + } + } +} + +/* case6.css */ + +================================================================================ +TestCSSAtImportConditionsAtLayerBundleAlternatingLayerInFile +---------- /out/case1.css ---------- +/* a.css */ +@layer first { + body { + color: red; + } +} + +/* case1.css */ + +---------- /out/case2.css ---------- +/* a.css */ +@layer first; + +/* b.css */ +@layer last { + body { + color: green; + } +} + +/* a.css */ +@layer first { + body { + color: red; + } +} + +/* case2.css */ + +---------- /out/case3.css ---------- +/* a.css */ +@layer first; + +/* b.css */ +@layer last; + +/* a.css */ +@layer first { + body { + color: red; + } +} + +/* b.css */ +@layer last { + body { + color: green; + } +} + +/* case3.css */ + +---------- /out/case4.css ---------- +/* a.css */ +@layer first; + +/* b.css */ +@layer last { + body { + color: green; + } +} + +/* a.css */ +@layer first { + body { + color: red; + } +} + +/* case4.css */ + +---------- /out/case5.css ---------- +/* a.css */ +@layer first; + +/* b.css */ +@layer last; + +/* a.css */ +@layer first { + body { + color: red; + } +} + +/* b.css */ +@layer last { + body { + color: green; + } +} + +/* case5.css */ + +---------- /out/case6.css ---------- +/* a.css */ +@layer first; + +/* b.css */ +@layer last { + body { + color: green; + } +} + +/* a.css */ +@layer first { + body { + color: red; + } +} + +/* case6.css */ + +================================================================================ +TestCSSAtImportConditionsAtLayerBundleAlternatingLayerOnImport +---------- /out/case1.css ---------- +/* a.css */ +@layer first { + body { + color: red; + } +} + +/* case1.css */ + +---------- /out/case2.css ---------- +/* a.css */ +@layer first; + +/* b.css */ +@layer last { + body { + color: green; + } +} + +/* a.css */ +@layer first { + body { + color: red; + } +} + +/* case2.css */ + +---------- /out/case3.css ---------- +/* a.css */ +@layer first; + +/* b.css */ +@layer last; + +/* a.css */ +@layer first { + body { + color: red; + } +} + +/* b.css */ +@layer last { + body { + color: green; + } +} + +/* case3.css */ + +---------- /out/case4.css ---------- +/* a.css */ +@layer first; + +/* b.css */ +@layer last { + body { + color: green; + } +} + +/* a.css */ +@layer first { + body { + color: red; + } +} + +/* case4.css */ + +---------- /out/case5.css ---------- +/* a.css */ +@layer first; + +/* b.css */ +@layer last; + +/* a.css */ +@layer first { + body { + color: red; + } +} + +/* b.css */ +@layer last { + body { + color: green; + } +} + +/* case5.css */ + +---------- /out/case6.css ---------- +/* a.css */ +@layer first; + +/* b.css */ +@layer last { + body { + color: green; + } +} + +/* a.css */ +@layer first { + body { + color: red; + } +} + +/* case6.css */ + ================================================================================ TestCSSAtImportConditionsBundle ---------- /out.css ---------- @@ -93,7 +465,6 @@ TestCSSAtImportConditionsBundle @import "http://example.com/foo.css" layer; @import "http://example.com/foo.css" layer(layer-name); @import "http://example.com/foo.css" layer(layer-name) supports(supports-condition); -@import "http://example.com/foo.css" layer(layer-name) supports(supports-condition) list-of-media-queries; @import "http://example.com/foo.css" layer(layer-name) list-of-media-queries; @import "http://example.com/foo.css" supports(supports-condition); @import "http://example.com/foo.css" list-of-media-queries; @@ -126,17 +497,6 @@ body { } } -/* foo.css */ -@media list-of-media-queries { - @supports (supports-condition) { - @layer layer-name { - body { - color: red; - } - } - } -} - /* foo.css */ @media list-of-media-queries { @layer layer-name { @@ -153,15 +513,6 @@ body { } } -/* foo.css */ -@media list-of-media-queries { - @supports (supports-condition) { - body { - color: red; - } - } -} - /* foo.css */ @media list-of-media-queries { body { @@ -360,11 +711,7 @@ TestCSSAtImportConditionsFromExternalRepo ---------- /out/at-layer/001/style.css ---------- /* at-layer/001/a.css */ -@layer a { - .box { - background-color: red; - } -} +@layer a; /* at-layer/001/b.css */ @layer b { @@ -385,11 +732,7 @@ TestCSSAtImportConditionsFromExternalRepo ---------- /out/at-layer/002/style.css ---------- /* at-layer/002/a.css */ @media print { - @layer a { - .box { - background-color: green; - } - } + @layer a; } /* at-layer/002/b.css */ @@ -409,6 +752,9 @@ TestCSSAtImportConditionsFromExternalRepo /* at-layer/002/style.css */ ---------- /out/at-layer/003/style.css ---------- +/* at-layer/003/a.css */ +@layer a; + /* at-layer/003/b.css */ @layer b { .box { diff --git a/internal/css_ast/css_ast.go b/internal/css_ast/css_ast.go index b606142ee99..f0d7450286e 100644 --- a/internal/css_ast/css_ast.go +++ b/internal/css_ast/css_ast.go @@ -34,6 +34,10 @@ type AST struct { LocalScope map[string]ast.LocRef GlobalScope map[string]ast.LocRef Composes map[ast.Ref]*Composes + + // This contains all layer names in the file. It can be used to replace the + // layer-related side effects of importing this file. + Layers [][]string } type Composes struct { diff --git a/internal/css_parser/css_parser.go b/internal/css_parser/css_parser.go index 8e0581ab7f0..2b17bab45fb 100644 --- a/internal/css_parser/css_parser.go +++ b/internal/css_parser/css_parser.go @@ -30,6 +30,9 @@ type parser struct { globalScope map[string]ast.LocRef nestingWarnings map[logger.Loc]struct{} tracker logger.LineColumnTracker + layers [][]string + enclosingLayer []string + anonLayerCount int index int end int legalCommentIndex int @@ -149,6 +152,7 @@ func Parse(log logger.Log, source logger.Source, options Options) css_ast.AST { LocalScope: p.localScope, GlobalScope: p.globalScope, Composes: p.composes, + Layers: p.layers, } } @@ -344,6 +348,20 @@ func (p *parser) symbolForName(loc logger.Loc, name string) ast.LocRef { return entry } +func (p *parser) recordAtLayerRule(layers [][]string) { + if p.anonLayerCount > 0 { + return + } + + for _, layer := range layers { + if len(p.enclosingLayer) > 0 { + clone := make([]string, 0, len(p.enclosingLayer)+len(layer)) + layer = append(append(clone, p.enclosingLayer...), layer...) + } + p.layers = append(p.layers, layer) + } +} + type ruleContext struct { isTopLevel bool parseSelectors bool @@ -1388,6 +1406,13 @@ abortRuleParser: // Read the optional block matchingLoc := p.current().Range.Loc if len(names) <= 1 && p.eat(css_lexer.TOpenBrace) { + p.recordAtLayerRule(names) + oldEnclosingLayer := p.enclosingLayer + if len(names) == 1 { + p.enclosingLayer = append(p.enclosingLayer, names[0]...) + } else { + p.anonLayerCount++ + } var rules []css_ast.Rule if context.isDeclarationList { rules = p.parseListOfDeclarations(listOfDeclarationsOpts{ @@ -1398,6 +1423,10 @@ abortRuleParser: parseSelectors: true, }) } + if len(names) != 1 { + p.anonLayerCount-- + } + p.enclosingLayer = oldEnclosingLayer closeBraceLoc := p.current().Range.Loc if !p.expectWithMatchingLoc(css_lexer.TCloseBrace, matchingLoc) { closeBraceLoc = logger.Loc{} @@ -1407,6 +1436,7 @@ abortRuleParser: // Handle lack of a block if len(names) >= 1 && p.eat(css_lexer.TSemicolon) { + p.recordAtLayerRule(names) return css_ast.Rule{Loc: atRange.Loc, Data: &css_ast.RAtLayer{Names: names}} } @@ -1414,11 +1444,13 @@ abortRuleParser: switch p.current().Kind { case css_lexer.TEndOfFile: p.expect(css_lexer.TSemicolon) + p.recordAtLayerRule(names) return css_ast.Rule{Loc: atRange.Loc, Data: &css_ast.RAtLayer{Names: names}} case css_lexer.TCloseBrace: p.expect(css_lexer.TSemicolon) if !context.isTopLevel { + p.recordAtLayerRule(names) return css_ast.Rule{Loc: atRange.Loc, Data: &css_ast.RAtLayer{Names: names}} } diff --git a/internal/linker/linker.go b/internal/linker/linker.go index 2f470914b8a..ef4b59dfca0 100644 --- a/internal/linker/linker.go +++ b/internal/linker/linker.go @@ -3322,16 +3322,15 @@ func (c *linkerContext) findImportedCSSFilesInJSOrder(entryPoint uint32) (order } type cssImportOrder struct { - sourceIndex uint32 conditions []css_ast.ImportConditions conditionImportRecords []ast.ImportRecord + sourceIndex uint32 + isRedundantDueTo ast.Index32 } -// CSS files are traversed in depth-first reversed reverse preorder. This is -// because unlike JavaScript import statements, CSS "@import" rules are -// evaluated every time instead of just the first time. However, evaluating a -// CSS file multiple times is equivalent to evaluating it once at the last -// location. So we drop all but the last evaluation in the order. +// CSS files are traversed in depth-first postorder just like JavaScript. But +// unlike JavaScript import statements, CSS "@import" rules are evaluated every +// time instead of just the first time. // // A // / \ @@ -3340,56 +3339,53 @@ type cssImportOrder struct { // D // // If A imports B and then C, B imports D, and C imports D, then the CSS -// traversal order is B D C A. +// traversal order is D B D C A. +// +// However, evaluating a CSS file multiple times is sort of equivalent to +// evaluating it once at the last location. So we basically drop all but the +// last evaluation in the order. +// +// The only exception to this is "@layer". Evaluating a CSS file multiple +// times is sort of equivalent to evaluating it once at the first location +// as far as "@layer" is concerned. So we may in some cases keep both the +// first and and last locations and only write out the "@layer" information +// for the first location. func (c *linkerContext) findImportedFilesInCSSOrder(entryPoints []uint32) (externalOrder []externalImportCSS, internalOrder []cssImportOrder) { - type externalImportsCSS struct { - conditions []css_ast.ImportConditions - } - - externals := make(map[logger.Path]externalImportsCSS) - var visit func(uint32, map[uint32]bool, []css_ast.ImportConditions, []ast.ImportRecord) + var visit func(uint32, []uint32, []css_ast.ImportConditions, []ast.ImportRecord) // Include this file and all files it imports visit = func( sourceIndex uint32, - visited map[uint32]bool, + visited []uint32, wrappingConditions []css_ast.ImportConditions, wrappingImportRecords []ast.ImportRecord, ) { - if visited[sourceIndex] { - return + // The CSS specification strangely does not describe what to do when there + // is a cycle. So we are left with reverse-engineering the behavior from a + // real browser. Here's what the WebKit code base has to say about this: + // + // "Check for a cycle in our import chain. If we encounter a stylesheet + // in our parent chain with the same URL, then just bail." + // + // So that's what we do here. See "StyleRuleImport::requestStyleSheet()" in + // WebKit for more information. + for _, visitedSourceIndex := range visited { + if visitedSourceIndex == sourceIndex { + return + } } - visited[sourceIndex] = true + visited = append(visited, sourceIndex) repr := c.graph.Files[sourceIndex].InputFile.Repr.(*graph.CSSRepr) topLevelRules := repr.AST.Rules - // Iterate in reverse preorder (will be reversed again later) - internalOrder = append(internalOrder, cssImportOrder{ - sourceIndex: sourceIndex, - conditions: wrappingConditions, - conditionImportRecords: wrappingImportRecords, - }) - - // Iterate in the inverse order of "composes" directives. Note that the - // order doesn't matter for these because the output order is explicitly - // undefined in the specification. - records := repr.AST.ImportRecords - for i := len(records) - 1; i >= 0; i-- { - if record := &records[i]; record.Kind == ast.ImportComposesFrom && record.SourceIndex.IsValid() { - visit(record.SourceIndex.GetIndex(), visited, wrappingConditions, wrappingImportRecords) - } - } - - // Iterate in the inverse order of top-level "@import" rules - outer: - for i := len(topLevelRules) - 1; i >= 0; i-- { - if atImport, ok := topLevelRules[i].Data.(*css_ast.RAtImport); ok { + // Iterate over the top-level "@import" rules + for _, rule := range topLevelRules { + if atImport, ok := rule.Data.(*css_ast.RAtImport); ok { record := &repr.AST.ImportRecords[atImport.ImportRecordIndex] // Follow internal dependencies if record.SourceIndex.IsValid() { - nestedVisited := visited nestedConditions := wrappingConditions nestedImportRecords := wrappingImportRecords @@ -3397,10 +3393,6 @@ func (c *linkerContext) findImportedFilesInCSSOrder(entryPoints []uint32) (exter // imported stylesheet subtree is wrapped in all of the conditions if atImport.ImportConditions != nil { // Fork our state - nestedVisited = make(map[uint32]bool) - for sourceIndex := range visited { - nestedVisited[sourceIndex] = true - } nestedConditions = append([]css_ast.ImportConditions{}, nestedConditions...) nestedImportRecords = append([]ast.ImportRecord{}, nestedImportRecords...) @@ -3410,67 +3402,17 @@ func (c *linkerContext) findImportedFilesInCSSOrder(entryPoints []uint32) (exter nestedConditions = append(nestedConditions, conditions) } - visit(record.SourceIndex.GetIndex(), nestedVisited, nestedConditions, nestedImportRecords) + visit(record.SourceIndex.GetIndex(), visited, nestedConditions, nestedImportRecords) continue } // Record external dependencies if (record.Flags & ast.WasLoadedWithEmptyLoader) == 0 { - external := externals[record.Path] - - // This is stored as a pointer to save space. But it's easier to - // compare against if we don't have to test for nil. So convert - // it from a pointer to a value. - var before css_ast.ImportConditions - if atImport.ImportConditions != nil { - before = *atImport.ImportConditions - } - - // Skip this rule if a later rule masks it. Note that we avoid - // skipping rules with layers because this code tries to keep - // the last instance of each import (since usually that's the - // only one that matters in CSS) but layers take effect for the - // first instance instead of the last. - if len(before.Layers) == 0 { - for _, after := range external.conditions { - if len(after.Layers) > 0 { - continue - } - - sameSupports := css_ast.TokensEqualIgnoringWhitespace(before.Supports, after.Supports) - sameMedia := css_ast.TokensEqualIgnoringWhitespace(before.Media, after.Media) - - // If the import conditions are exactly equal, then only keep - // the later one. The earlier one will have no effect. - if sameSupports && sameMedia { - continue outer - } - - // If the media conditions are exactly equal and the later one - // doesn't have any supports conditions, then the later one will - // apply in all cases where the earlier one applies. - if sameMedia && len(after.Supports) == 0 { - continue outer - } - - // If the supports conditions are exactly equal and the later one - // doesn't have any media conditions, then the later one will - // apply in all cases where the earlier one applies. - if sameSupports && len(after.Media) == 0 { - continue outer - } - } - } - - external.conditions = append(external.conditions, before) - externals[record.Path] = external - var conditions css_ast.ImportConditions var importRecords []ast.ImportRecord if atImport.ImportConditions != nil { conditions, importRecords = atImport.ImportConditions.CloneWithImportRecords(repr.AST.ImportRecords, importRecords) } - externalOrder = append(externalOrder, externalImportCSS{ path: record.Path, conditions: conditions, @@ -3479,25 +3421,255 @@ func (c *linkerContext) findImportedFilesInCSSOrder(entryPoints []uint32) (exter } } } + + // Iterate over the "composes" directives. Note that the order doesn't + // matter for these because the output order is explicitly undefined + // in the specification. + for _, record := range repr.AST.ImportRecords { + if record.Kind == ast.ImportComposesFrom && record.SourceIndex.IsValid() { + visit(record.SourceIndex.GetIndex(), visited, wrappingConditions, wrappingImportRecords) + } + } + + // Accumulate imports in depth-first postorder + internalOrder = append(internalOrder, cssImportOrder{ + sourceIndex: sourceIndex, + conditions: wrappingConditions, + conditionImportRecords: wrappingImportRecords, + }) } // Include all files reachable from any entry point - visited := make(map[uint32]bool) - for i := len(entryPoints) - 1; i >= 0; i-- { - visit(entryPoints[i], visited, nil, nil) + var visited [16]uint32 // Preallocate some space for the visited set + for _, sourceIndex := range entryPoints { + visit(sourceIndex, visited[:], nil, nil) } - // Reverse the order afterward when traversing in CSS order - for i, j := 0, len(internalOrder)-1; i < j; i, j = i+1, j-1 { - internalOrder[i], internalOrder[j] = internalOrder[j], internalOrder[i] + // Optimize the internal import order + { + internalDuplicates := make(map[uint32][]int) + + // If there are duplicate entries, mark all but the last entry as redundant. + // This uses the term "redundant" because for normal CSS that does not use + // "@layer", only the last entry actually has any effect. + nextInternal: + for i := len(internalOrder) - 1; i >= 0; i-- { + order := internalOrder[i] + duplicates := internalDuplicates[order.sourceIndex] + for _, j := range duplicates { + if isConditionalImportRedundant(order.conditions, internalOrder[j].conditions) { + internalOrder[i].isRedundantDueTo = ast.MakeIndex32(uint32(j)) + continue nextInternal + } + } + internalDuplicates[order.sourceIndex] = append(duplicates, i) + } + + // Remove duplicate redundant entries from the list. We always keep the + // first redundant entry in case it contains one or more "@layer" rules. + // In CSS "@layer" always takes effect in the first location it appears. + // But we only care about at most the first and last duplicate entries. + redundantDuplicates := make(map[uint32]bool) + end := 0 + for i := 0; i < len(internalOrder); i++ { + order := internalOrder[i] + + // Overwrite the previous entry if it's the same as this one + if end > 0 { + if prev := internalOrder[end-1].isRedundantDueTo; prev.IsValid() && (prev == order.isRedundantDueTo || prev.GetIndex() == uint32(i)) { + delete(redundantDuplicates, order.isRedundantDueTo.GetIndex()) + end-- + } + } + + // Redundant entries may be removed + if order.isRedundantDueTo.IsValid() { + // Remove this redundant entry if it's a duplicate of a previous one + if redundantDuplicates[order.isRedundantDueTo.GetIndex()] { + continue + } + + // Remove this redundant entry either if it has no named layers + // or if it's wrapped in an anonymous layer without a name + hasNamedLayers := len(c.graph.Files[order.sourceIndex].InputFile.Repr.(*graph.CSSRepr).AST.Layers) > 0 + hasAnonymousLayer := false + for _, conditions := range order.conditions { + if len(conditions.Layers) == 1 { + if t := conditions.Layers[0]; t.Children == nil || len(*t.Children) == 0 { + hasAnonymousLayer = true + } else { + hasNamedLayers = true + } + } + } + if !hasNamedLayers || hasAnonymousLayer { + continue + } + + // Remove further copies of this redundant entry + redundantDuplicates[order.isRedundantDueTo.GetIndex()] = true + } + + internalOrder[end] = order + end++ + } + internalOrder = internalOrder[:end] } - for i, j := 0, len(externalOrder)-1; i < j; i, j = i+1, j-1 { - externalOrder[i], externalOrder[j] = externalOrder[j], externalOrder[i] + + // Optimize the external import order + { + externalDuplicates := make(map[logger.Path][]int) + isRedundantDueTo := make([]ast.Index32, len(externalOrder)) + + // If there are duplicate entries, mark all but the last entry as redundant. + // This uses the term "redundant" because for normal CSS that does not use + // "@layer", only the last entry actually has any effect. + nextExternal: + for i := len(externalOrder) - 1; i >= 0; i-- { + order := externalOrder[i] + duplicates := externalDuplicates[order.path] + for _, j := range duplicates { + if isConditionalImportRedundant([]css_ast.ImportConditions{order.conditions}, []css_ast.ImportConditions{externalOrder[j].conditions}) { + isRedundantDueTo[i] = ast.MakeIndex32(uint32(j)) + continue nextExternal + } + } + externalDuplicates[order.path] = append(duplicates, i) + } + + // Remove duplicate redundant entries from the list. We always keep the + // first redundant entry in case it contains one or more "@layer" rules. + // In CSS "@layer" always takes effect in the first location it appears. + // But we only care about at most the first and last duplicate entries. + redundantDuplicates := make(map[uint32]bool) + end := 0 + for i := 0; i < len(externalOrder); i++ { + order := externalOrder[i] + redundant := isRedundantDueTo[i] + + // Overwrite the previous entry if it's the same as this one + if end > 0 { + if prev := isRedundantDueTo[end-1]; prev.IsValid() && (prev == redundant || prev.GetIndex() == uint32(i)) { + delete(redundantDuplicates, redundant.GetIndex()) + end-- + } + } + + // Redundant entries may be removed + if redundant.IsValid() { + // Remove this redundant entry if it's a duplicate of a previous one + if redundantDuplicates[redundant.GetIndex()] { + continue + } + + // Remove this redundant entry if it has no layers. THIS ASSUMES THAT + // EXTERNAL IMPORTS DO NOT CONTAIN "@layer" INFORMATION. While this is not + // necessarily a correct assumption, there are other ordering things that + // are also not correct about external "@import" rules when bundling (e.g. + // CSS doesn't allow you to put an "@import" rule in the middle of a file + // so we have to hoist them all to the top) so we don't worry about this. + if len(order.conditions.Layers) == 0 { + continue + } + + // Remove further copies of this redundant entry + redundantDuplicates[redundant.GetIndex()] = true + } + + externalOrder[end] = order + end++ + } + externalOrder = externalOrder[:end] } return } +// Given two "@import" rules for the same source index (an earlier one and a +// later one), the earlier one is masked by the later one if the later one's +// condition list is a prefix of the earlier one's condition list. +// +// For example: +// +// // entry.css +// @import "foo.css" supports(display: flex); +// @import "bar.css" supports(display: flex); +// +// // foo.css +// @import "lib.css" screen; +// +// // bar.css +// @import "lib.css"; +// +// When we bundle this code we'll get an import order as follows: +// +// 1. lib.css [supports(display: flex), screen] +// 2. foo.css [supports(display: flex)] +// 3. lib.css [supports(display: flex)] +// 4. bar.css [supports(display: flex)] +// 5. entry.css [] +// +// For "lib.css", the entry with the conditions [supports(display: flex)] should +// make the entry with the conditions [supports(display: flex), screen] redundant. +// +// Note that all of this deliberately ignores the existance of "@layer" because +// that is handled separately. All of this is only for handling unlayered styles. +func isConditionalImportRedundant(earlier []css_ast.ImportConditions, later []css_ast.ImportConditions) bool { + if len(later) > len(earlier) { + return false + } + + for i := 0; i < len(later); i++ { + a := earlier[i] + b := later[i] + + // Only compare "@supports" and "@media" if "@layers" is equal + if css_ast.TokensEqualIgnoringWhitespace(a.Layers, b.Layers) { + sameSupports := css_ast.TokensEqualIgnoringWhitespace(a.Supports, b.Supports) + sameMedia := css_ast.TokensEqualIgnoringWhitespace(a.Media, b.Media) + + // If the import conditions are exactly equal, then only keep + // the later one. The earlier one is redundant. Example: + // + // @import "foo.css" layer(abc) supports(display: flex) screen; + // @import "foo.css" layer(abc) supports(display: flex) screen; + // + // The later one makes the earlier one redundant. + if sameSupports && sameMedia { + continue + } + + // If the media conditions are exactly equal and the later one + // doesn't have any supports conditions, then the later one will + // apply in all cases where the earlier one applies. Example: + // + // @import "foo.css" layer(abc) supports(display: flex) screen; + // @import "foo.css" layer(abc) screen; + // + // The later one makes the earlier one redundant. + if sameMedia && len(b.Supports) == 0 { + continue + } + + // If the supports conditions are exactly equal and the later one + // doesn't have any media conditions, then the later one will + // apply in all cases where the earlier one applies. Example: + // + // @import "foo.css" layer(abc) supports(display: flex) screen; + // @import "foo.css" layer(abc) supports(display: flex); + // + // The later one makes the earlier one redundant. + if sameSupports && len(b.Media) == 0 { + continue + } + } + + return false + } + + return true +} + func (c *linkerContext) computeChunks() { c.timer.Begin("Compute chunks") defer c.timer.End("Compute chunks") @@ -5685,18 +5857,24 @@ func (c *linkerContext) generateChunkCSS(chunkIndex int, chunkWaitGroup *sync.Wa entry := chunkRepr.filesInChunkInOrder[i] file := &c.graph.Files[entry.sourceIndex] ast := file.InputFile.Repr.(*graph.CSSRepr).AST - - // Filter out "@charset" and "@import" rules - rules := make([]css_ast.Rule, 0, len(ast.Rules)) - for _, rule := range ast.Rules { - switch rule.Data.(type) { - case *css_ast.RAtCharset: - compileResults[i].hasCharset = true - continue - case *css_ast.RAtImport: - continue + var rules []css_ast.Rule + + if !entry.isRedundantDueTo.IsValid() { + // Filter out "@charset" and "@import" rules + rules = make([]css_ast.Rule, 0, len(ast.Rules)) + for _, rule := range ast.Rules { + switch rule.Data.(type) { + case *css_ast.RAtCharset: + compileResults[i].hasCharset = true + continue + case *css_ast.RAtImport: + continue + } + rules = append(rules, rule) } - rules = append(rules, rule) + } else if len(ast.Layers) > 0 { + // Only include "@layer" information for redundant import order entries + rules = []css_ast.Rule{{Data: &css_ast.RAtLayer{Names: ast.Layers}}} } for i := len(entry.conditions) - 1; i >= 0; i-- {