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

fix(glob): css imports injecting a ?used query to export the css string #6949

Merged
merged 9 commits into from Feb 20, 2022
Merged

fix(glob): css imports injecting a ?used query to export the css string #6949

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
f4b0802
test: test for css inconsistemcy
poyoho Feb 16, 2022
4385e12
feat: import css should be emtry string
poyoho Feb 17, 2022
8645fb6
feat: css imports injecting a ?used query to export the css string
poyoho Feb 17, 2022
931d0b9
fix: expect
poyoho Feb 17, 2022
2e88d35
fix: test
poyoho Feb 17, 2022
bff7271
fix: test
poyoho Feb 17, 2022
51b28ef
chore: something code suggested
poyoho Feb 20, 2022
420817e
chore: extract reused code
poyoho Feb 20, 2022
85de1d1
chore: update
patak-dev Feb 20, 2022
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
8 changes: 8 additions & 0 deletions packages/playground/css/__tests__/css.spec.ts
View file
Open in desktop
Expand Up @@ -13,6 +13,14 @@ import {

// note: tests should retrieve the element at the beginning of test and reuse it
// in later assertions to ensure CSS HMR doesn't reload the page
test('imported css', async () => {
const css = await page.textContent('.imported-css')
expect(css).toContain('.imported {')
const glob = await page.textContent('.imported-css-glob')
expect(glob).toContain('.dir-import')
const globEager = await page.textContent('.imported-css-globEager')
expect(globEager).toContain('.dir-import')
})

test('linked css', async () => {
const linked = await page.$('.linked')
Expand Down
3 changes: 3 additions & 0 deletions packages/playground/css/glob-import/bar.css
View file
Open in desktop
@@ -0,0 +1,3 @@
.dir-import-2 {
color: grey;
}
3 changes: 3 additions & 0 deletions packages/playground/css/glob-import/foo.css
View file
Open in desktop
@@ -0,0 +1,3 @@
.dir-import {
color: grey;
}
2 changes: 2 additions & 0 deletions packages/playground/css/index.html
View file
Open in desktop
Expand Up @@ -12,6 +12,8 @@ <h1>CSS</h1>
</p>
<p>Imported css string:</p>
<pre class="imported-css"></pre>
<pre class="imported-css-glob"></pre>
<pre class="imported-css-globEager"></pre>

<p class="postcss">
<span class="nesting">PostCSS nesting plugin: this should be pink</span>
Expand Down
7 changes: 7 additions & 0 deletions packages/playground/css/main.js
View file
Open in desktop
Expand Up @@ -68,3 +68,10 @@ if (import.meta.env.DEV) {
// inlined
import inlined from './inlined.css?inline'
text('.inlined-code', inlined)

const glob = import.meta.glob('./glob-import/*.css')
poyoho marked this conversation as resolved.
Show resolved Hide resolved
Promise.all(Object.keys(glob).map((key) => glob[key]())).then((res) => {
text('.imported-css-glob', JSON.stringify(res, null, 2))
})
const globEager = import.meta.globEager('./glob-import/*.css')
text('.imported-css-globEager', JSON.stringify(globEager, null, 2))
10 changes: 8 additions & 2 deletions packages/vite/src/node/importGlob.ts
View file
Open in desktop
@@ -1,3 +1,4 @@
import { isCSSRequest } from './plugins/css'
poyoho marked this conversation as resolved.
Show resolved Hide resolved
import path from 'path'
import { promises as fsp } from 'fs'
import glob from 'fast-glob'
Expand Down Expand Up @@ -94,12 +95,17 @@ export async function transformImportGlob(
)},`
} else if (isEager) {
Copy link
Member

Choose a reason for hiding this comment

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

I think the code is going to be more clear if we change it to

} else {
  const importeeUrl = isCSSRequest(importee) ? `${importee}?used` : importee
  if (isEager) {
    ...
  }
  else {
    ...
  }
}

We can avoid repeating the condition, and the main else makes it more evident that we are dealing with the non-raw case in both branches.

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 seem to have found a pattern that when a fragment is reused it should be extracted. 😀

Copy link
Member

Choose a reason for hiding this comment

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

ha, not always, early abstraction is sometimes worse than duplicating code. But in this case, the variable applied to both branches when it was non-raw. I moved inside the else case, as it is not used in the raw branch.

const identifier = `__glob_${importIndex}_${i}`
// css imports injecting a ?used query to export the css string
importsString += `import ${
isEagerDefault ? `` : `* as `
}${identifier} from ${JSON.stringify(importee)};`
}${identifier} from ${JSON.stringify(
isCSSRequest(importee) ? `${importee}?used` : importee
)};`
entries += ` ${JSON.stringify(file)}: ${identifier},`
} else {
let imp = `import(${JSON.stringify(importee)})`
let imp = `import(${JSON.stringify(
isCSSRequest(importee) ? `${importee}?used` : importee
)})`
if (!normalizeUrl && preload) {
imp =
`(${isModernFlag}` +
Expand Down