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

perf(css): hoist at rules with regex #7691

Merged
merged 3 commits into from Apr 12, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
36 changes: 35 additions & 1 deletion packages/vite/src/node/__tests__/plugins/css.spec.ts
@@ -1,4 +1,4 @@
import { cssUrlRE, cssPlugin } from '../../plugins/css'
import { cssUrlRE, cssPlugin, hoistAtRules } from '../../plugins/css'
import { resolveConfig } from '../../config'
import fs from 'fs'
import path from 'path'
Expand Down Expand Up @@ -114,3 +114,37 @@ describe('css path resolutions', () => {
mockFs.mockReset()
})
})

describe('hoist @ rules', () => {
test('hoist @import', async () => {
const css = `.foo{color:red;}@import "bla";`
const result = await hoistAtRules(css)
expect(result).toBe(`@import "bla";.foo{color:red;}`)
})

test('hoist @import with semicolon in quotes', async () => {
const css = `.foo{color:red;}@import "bla;bar";`
const result = await hoistAtRules(css)
expect(result).toBe(`@import "bla;bar";.foo{color:red;}`)
})

test('hoist @charset', async () => {
const css = `.foo{color:red;}@charset "utf-8";`
const result = await hoistAtRules(css)
expect(result).toBe(`@charset "utf-8";.foo{color:red;}`)
})

test('hoist one @charset only', async () => {
const css = `.foo{color:red;}@charset "utf-8";@charset "utf-8";`
const result = await hoistAtRules(css)
expect(result).toBe(`@charset "utf-8";.foo{color:red;}`)
})

test('hoist @import and @charset', async () => {
const css = `.foo{color:red;}@import "bla";@charset "utf-8";.bar{color:grren;}@import "baz";`
const result = await hoistAtRules(css)
expect(result).toBe(
`@charset "utf-8";@import "bla";@import "baz";.foo{color:red;}.bar{color:grren;}`
)
})
})
48 changes: 20 additions & 28 deletions packages/vite/src/node/plugins/css.ts
Expand Up @@ -1106,36 +1106,28 @@ async function minifyCSS(css: string, config: ResolvedConfig) {
return code
}

// #1845
// CSS @import can only appear at top of the file. We need to hoist all @import
// to top when multiple files are concatenated.
// #6333
// CSS @charset must be the top-first in the file, hoist to top too
async function hoistAtRules(css: string) {
const postcss = await import('postcss')
return (await postcss.default([AtRuleHoistPlugin]).process(css)).css
}

const AtRuleHoistPlugin: PostCSS.PluginCreator<any> = () => {
return {
postcssPlugin: 'vite-hoist-at-rules',
Once(root) {
const imports: PostCSS.AtRule[] = []
let charset: PostCSS.AtRule | undefined
root.walkAtRules((rule) => {
if (rule.name === 'import') {
// record in reverse so that can simply prepend to preserve order
imports.unshift(rule)
} else if (!charset && rule.name === 'charset') {
charset = rule
}
})
imports.forEach((i) => root.prepend(i))
if (charset) root.prepend(charset)
export async function hoistAtRules(css: string) {
const s = new MagicString(css)
// #1845
// CSS @import can only appear at top of the file. We need to hoist all @import
// to top when multiple files are concatenated.
// match until semicolon that's not in quotes
s.replace(/@import\s*(?:"[^"]*"|'[^']*'|[^;]*).*?;/gm, (match) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@import /* comment; */ url( "baz") /* com;ment */; @import seems to support comments, will break the match.

I just found out that there is also a border issue. "word\"" 🙈

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those are real edge cases 🥲 It looks tricky to use emptyString() with MagicString.replace though, and doing so would make another copy of the string, which brings in some memory overhead. I feel like we could ignore them for now since there are unlikely to be instances of this in practice.

Otherwise, it may be simpler to go back to the postcss way 🤔

Copy link
Member

@poyoho poyoho Apr 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes this edge case, but if you want to avoid it, you can use the following code. 😊

s.replace(/@import\s*(?:"[^"]*"|'[^']*'|[^;]*).*?;|\/\*(.|[\r\n])*?\*\/|\/\/.*/gm, (match) => {
  if (match[0] === '/') return match
  ...
})

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried the regex and MagicString doesn't seem to like it: Cannot split a chunk that has already been edited (0:38 – "/* semicolon; */"). We may have to resort to not using .replace if we want to robust-ify the regex.

s.appendLeft(0, match)
return ''
})
// #6333
// CSS @charset must be the top-first in the file, hoist the first to top
let foundCharset = false
s.replace(/@charset\s*(?:"[^"]*"|'[^']*'|[^;]*).*?;/gm, (match) => {
if (!foundCharset) {
s.prepend(match)
foundCharset = true
}
}
return ''
})
return s.toString()
}
AtRuleHoistPlugin.postcss = true

// Preprocessor support. This logic is largely replicated from @vue/compiler-sfc

Expand Down