From f4562d7093993a375920725dce578a67b5b5cb25 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bartosz=20Go=C5=9Bci=C5=84ski?= Date: Sat, 3 Sep 2022 00:00:25 +0200 Subject: [PATCH 1/4] fix: don't duplicate styles with dynamic import (fix #9967) --- .../src/node/plugins/importAnalysisBuild.ts | 20 ++++- .../__tests__/css-dynamic-import.spec.ts | 86 +++++++++++++++++++ .../css-dynamic-import/__tests__/serve.ts | 10 +++ playground/css-dynamic-import/dynamic.css | 3 + playground/css-dynamic-import/dynamic.js | 6 ++ playground/css-dynamic-import/index.html | 3 + playground/css-dynamic-import/index.js | 5 ++ playground/css-dynamic-import/static.css | 3 + playground/css-dynamic-import/static.js | 3 + playground/test-utils.ts | 3 +- 10 files changed, 139 insertions(+), 3 deletions(-) create mode 100644 playground/css-dynamic-import/__tests__/css-dynamic-import.spec.ts create mode 100644 playground/css-dynamic-import/__tests__/serve.ts create mode 100644 playground/css-dynamic-import/dynamic.css create mode 100644 playground/css-dynamic-import/dynamic.js create mode 100644 playground/css-dynamic-import/index.html create mode 100644 playground/css-dynamic-import/index.js create mode 100644 playground/css-dynamic-import/static.css create mode 100644 playground/css-dynamic-import/static.js diff --git a/packages/vite/src/node/plugins/importAnalysisBuild.ts b/packages/vite/src/node/plugins/importAnalysisBuild.ts index d78e4c3be77ba7..aad13b62cfc85c 100644 --- a/packages/vite/src/node/plugins/importAnalysisBuild.ts +++ b/packages/vite/src/node/plugins/importAnalysisBuild.ts @@ -75,10 +75,26 @@ function preload( seen[dep] = true const isCss = dep.endsWith('.css') const cssSelector = isCss ? '[rel="stylesheet"]' : '' - // @ts-ignore check if the file is already preloaded by SSR markup - if (document.querySelector(`link[href="${dep}"]${cssSelector}`)) { + const isBaseRelative = !!importerUrl + + // check if the file is already preloaded by SSR markup + if (isBaseRelative) { + const separatorIdx = dep.lastIndexOf('/') + const shortDep = separatorIdx < 0 ? dep : dep.slice(separatorIdx + 1) + const linkSelector = `link[href$="${shortDep}"]${cssSelector}` + const links = document.querySelectorAll(linkSelector) + + for (let i = links.length - 1; i >= 0; i--) { + // When isBaseRelative is true then we have importerUrl and `dep` is + // already converted to an absolute URL by the assetsURL function. The + // `link.href` also has an absolute URL thanks to browser doing the + // work for us. See https://html.spec.whatwg.org/multipage/common-dom-interfaces.html#reflecting-content-attributes-in-idl-attributes:idl-domstring-5 + if (links[i].href === dep) return + } + } else if (document.querySelector(`link[href="${dep}"]${cssSelector}`)) { return } + // @ts-ignore const link = document.createElement('link') // @ts-ignore diff --git a/playground/css-dynamic-import/__tests__/css-dynamic-import.spec.ts b/playground/css-dynamic-import/__tests__/css-dynamic-import.spec.ts new file mode 100644 index 00000000000000..9a308efb8e8758 --- /dev/null +++ b/playground/css-dynamic-import/__tests__/css-dynamic-import.spec.ts @@ -0,0 +1,86 @@ +import type { InlineConfig } from 'vite' +import { build, createServer, preview } from 'vite' +import { expect, test } from 'vitest' +import { getColor, isBuild, isServe, page, ports, rootDir } from '~utils' + +const baseOptions = [ + { base: '', label: 'relative' }, + { base: '/', label: 'absolute' } +] + +const getConfig = (base: string): InlineConfig => ({ + base, + root: rootDir, + logLevel: 'silent', + preview: { port: ports['css/dynamic-import'] } +}) + +async function withBuild(base: string, fn: () => Promise) { + const config = getConfig(base) + await build(config) + const server = await preview(config) + + try { + await page.goto(server.resolvedUrls.local[0]) + await fn() + } finally { + server.httpServer.close() + } +} + +async function withServe(base: string, fn: () => Promise) { + const config = getConfig(base) + const server = await createServer(config) + await server.listen() + await new Promise((r) => setTimeout(r, 500)) + + try { + await page.goto(server.resolvedUrls.local[0]) + await fn() + } finally { + await server.close() + } +} + +async function getChunks() { + const links = await page.$$('link') + const hrefs = await Promise.all(links.map((l) => l.evaluate((el) => el.href))) + return hrefs.map((href) => { + // drop hash part from the file name + const [_, name, ext] = href.match(/assets\/([a-z]+)\..*?\.(.*)$/) + return `${name}.${ext}` + }) +} + +baseOptions.forEach(({ base, label }) => { + test.runIf(isBuild)( + `doesn't duplicate dynamically imported css files when built with ${label} base`, + async () => { + await withBuild(base, async () => { + await page.waitForSelector('.loaded', { state: 'attached' }) + + expect(await getColor('.css-dynamic-import')).toBe('green') + expect(await getChunks()).toEqual([ + 'index.css', + 'dynamic.js', + 'dynamic.css', + 'static.js', + 'index.js' + ]) + }) + } + ) + + test.runIf(isServe)( + `doesn't duplicate dynamically imported css files when served with ${label} base`, + async () => { + await withServe(base, async () => { + await page.waitForSelector('.loaded', { state: 'attached' }) + + expect(await getColor('.css-dynamic-import')).toBe('green') + // in serve there is no preloading + expect(await getChunks()).toEqual([]) + }) + } + ) +}) diff --git a/playground/css-dynamic-import/__tests__/serve.ts b/playground/css-dynamic-import/__tests__/serve.ts new file mode 100644 index 00000000000000..ae33c33a5db107 --- /dev/null +++ b/playground/css-dynamic-import/__tests__/serve.ts @@ -0,0 +1,10 @@ +// this is automatically detected by playground/vitestSetup.ts and will replace +// the default e2e test serve behavior + +// The server is started in the test, so we need to have a custom serve +// function or a default server will be created +export async function serve() { + return { + close: () => Promise.resolve() + } +} diff --git a/playground/css-dynamic-import/dynamic.css b/playground/css-dynamic-import/dynamic.css new file mode 100644 index 00000000000000..6212a63c31fa19 --- /dev/null +++ b/playground/css-dynamic-import/dynamic.css @@ -0,0 +1,3 @@ +.css-dynamic-import { + color: green; +} diff --git a/playground/css-dynamic-import/dynamic.js b/playground/css-dynamic-import/dynamic.js new file mode 100644 index 00000000000000..0d0aeb3aec229c --- /dev/null +++ b/playground/css-dynamic-import/dynamic.js @@ -0,0 +1,6 @@ +import './dynamic.css' + +export const lazyLoad = async () => { + await import('./static.js') + document.body.classList.add('loaded') +} diff --git a/playground/css-dynamic-import/index.html b/playground/css-dynamic-import/index.html new file mode 100644 index 00000000000000..d9f9fedbbda752 --- /dev/null +++ b/playground/css-dynamic-import/index.html @@ -0,0 +1,3 @@ +

This should be green

+ + diff --git a/playground/css-dynamic-import/index.js b/playground/css-dynamic-import/index.js new file mode 100644 index 00000000000000..9115d2b4aa007e --- /dev/null +++ b/playground/css-dynamic-import/index.js @@ -0,0 +1,5 @@ +import './static.js' + +import('./dynamic.js').then(async ({ lazyLoad }) => { + await lazyLoad() +}) diff --git a/playground/css-dynamic-import/static.css b/playground/css-dynamic-import/static.css new file mode 100644 index 00000000000000..4efb84fdfea550 --- /dev/null +++ b/playground/css-dynamic-import/static.css @@ -0,0 +1,3 @@ +.css-dynamic-import { + color: red; +} diff --git a/playground/css-dynamic-import/static.js b/playground/css-dynamic-import/static.js new file mode 100644 index 00000000000000..1688198fba4227 --- /dev/null +++ b/playground/css-dynamic-import/static.js @@ -0,0 +1,3 @@ +import './static.css' + +export const foo = 'foo' diff --git a/playground/test-utils.ts b/playground/test-utils.ts index c7c3288fafe2ff..c27e8ffe8285df 100644 --- a/playground/test-utils.ts +++ b/playground/test-utils.ts @@ -29,7 +29,8 @@ export const ports = { 'ssr-vue': 9604, 'ssr-webworker': 9605, 'css/postcss-caching': 5005, - 'css/postcss-plugins-different-dir': 5006 + 'css/postcss-plugins-different-dir': 5006, + 'css/dynamic-import': 5007 } export const hmrPorts = { 'optimize-missing-deps': 24680, From 3b094900d5fc8d642bcaa7878b6244b4649b0174 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bartosz=20Go=C5=9Bci=C5=84ski?= Date: Mon, 12 Sep 2022 21:24:14 +0200 Subject: [PATCH 2/4] fix: remove micro optimization --- packages/vite/src/node/plugins/importAnalysisBuild.ts | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/packages/vite/src/node/plugins/importAnalysisBuild.ts b/packages/vite/src/node/plugins/importAnalysisBuild.ts index aad13b62cfc85c..e6891515a6eda8 100644 --- a/packages/vite/src/node/plugins/importAnalysisBuild.ts +++ b/packages/vite/src/node/plugins/importAnalysisBuild.ts @@ -79,10 +79,9 @@ function preload( // check if the file is already preloaded by SSR markup if (isBaseRelative) { - const separatorIdx = dep.lastIndexOf('/') - const shortDep = separatorIdx < 0 ? dep : dep.slice(separatorIdx + 1) - const linkSelector = `link[href$="${shortDep}"]${cssSelector}` - const links = document.querySelectorAll(linkSelector) + const links = document.querySelectorAll( + `link${cssSelector}` + ) for (let i = links.length - 1; i >= 0; i--) { // When isBaseRelative is true then we have importerUrl and `dep` is From 82dea8ea774cbfb4455cc799dff00bc31b4e55b9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bartosz=20Go=C5=9Bci=C5=84ski?= Date: Mon, 12 Sep 2022 22:39:50 +0200 Subject: [PATCH 3/4] fix: precompute absoluteUrls in preload --- .../src/node/plugins/importAnalysisBuild.ts | 22 +++++++++---------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/packages/vite/src/node/plugins/importAnalysisBuild.ts b/packages/vite/src/node/plugins/importAnalysisBuild.ts index e6891515a6eda8..d5ca19f0141d54 100644 --- a/packages/vite/src/node/plugins/importAnalysisBuild.ts +++ b/packages/vite/src/node/plugins/importAnalysisBuild.ts @@ -65,6 +65,14 @@ function preload( return baseModule() } + const absoluteUrls = new Set() + const links = document.getElementsByTagName('link') + for (let i = links.length - 1; i >= 0; i--) { + // The `links[i].href` is an absolute URL thanks to browser doing the work + // for us. See https://html.spec.whatwg.org/multipage/common-dom-interfaces.html#reflecting-content-attributes-in-idl-attributes:idl-domstring-5 + absoluteUrls.add(links[i].href) + } + return Promise.all( deps.map((dep) => { // @ts-ignore @@ -79,17 +87,9 @@ function preload( // check if the file is already preloaded by SSR markup if (isBaseRelative) { - const links = document.querySelectorAll( - `link${cssSelector}` - ) - - for (let i = links.length - 1; i >= 0; i--) { - // When isBaseRelative is true then we have importerUrl and `dep` is - // already converted to an absolute URL by the assetsURL function. The - // `link.href` also has an absolute URL thanks to browser doing the - // work for us. See https://html.spec.whatwg.org/multipage/common-dom-interfaces.html#reflecting-content-attributes-in-idl-attributes:idl-domstring-5 - if (links[i].href === dep) return - } + // When isBaseRelative is true then we have `importerUrl` and `dep` is + // already converted to an absolute URL by the `assetsURL` function + if (absoluteUrls.has(dep)) return } else if (document.querySelector(`link[href="${dep}"]${cssSelector}`)) { return } From d199621fd4ec254708ed6cb04da7b278976ac713 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bartosz=20Go=C5=9Bci=C5=84ski?= Date: Tue, 13 Sep 2022 20:12:41 +0200 Subject: [PATCH 4/4] fix: differentiate between rel=preload and rel=stylesheet for CSS files --- .../src/node/plugins/importAnalysisBuild.ts | 15 +++-- .../__tests__/css-dynamic-import.spec.ts | 65 ++++++++++++++----- playground/css-dynamic-import/index.js | 5 ++ 3 files changed, 63 insertions(+), 22 deletions(-) diff --git a/packages/vite/src/node/plugins/importAnalysisBuild.ts b/packages/vite/src/node/plugins/importAnalysisBuild.ts index d5ca19f0141d54..ed8d8853e7680a 100644 --- a/packages/vite/src/node/plugins/importAnalysisBuild.ts +++ b/packages/vite/src/node/plugins/importAnalysisBuild.ts @@ -65,13 +65,7 @@ function preload( return baseModule() } - const absoluteUrls = new Set() const links = document.getElementsByTagName('link') - for (let i = links.length - 1; i >= 0; i--) { - // The `links[i].href` is an absolute URL thanks to browser doing the work - // for us. See https://html.spec.whatwg.org/multipage/common-dom-interfaces.html#reflecting-content-attributes-in-idl-attributes:idl-domstring-5 - absoluteUrls.add(links[i].href) - } return Promise.all( deps.map((dep) => { @@ -89,7 +83,14 @@ function preload( if (isBaseRelative) { // When isBaseRelative is true then we have `importerUrl` and `dep` is // already converted to an absolute URL by the `assetsURL` function - if (absoluteUrls.has(dep)) return + for (let i = links.length - 1; i >= 0; i--) { + const link = links[i] + // The `links[i].href` is an absolute URL thanks to browser doing the work + // for us. See https://html.spec.whatwg.org/multipage/common-dom-interfaces.html#reflecting-content-attributes-in-idl-attributes:idl-domstring-5 + if (link.href === dep && (!isCss || link.rel === 'stylesheet')) { + return + } + } } else if (document.querySelector(`link[href="${dep}"]${cssSelector}`)) { return } diff --git a/playground/css-dynamic-import/__tests__/css-dynamic-import.spec.ts b/playground/css-dynamic-import/__tests__/css-dynamic-import.spec.ts index 9a308efb8e8758..56757fc293dbba 100644 --- a/playground/css-dynamic-import/__tests__/css-dynamic-import.spec.ts +++ b/playground/css-dynamic-import/__tests__/css-dynamic-import.spec.ts @@ -12,7 +12,8 @@ const getConfig = (base: string): InlineConfig => ({ base, root: rootDir, logLevel: 'silent', - preview: { port: ports['css/dynamic-import'] } + preview: { port: ports['css/dynamic-import'] }, + build: { assetsInlineLimit: 0 } }) async function withBuild(base: string, fn: () => Promise) { @@ -42,14 +43,17 @@ async function withServe(base: string, fn: () => Promise) { } } -async function getChunks() { +async function getLinks() { const links = await page.$$('link') - const hrefs = await Promise.all(links.map((l) => l.evaluate((el) => el.href))) - return hrefs.map((href) => { - // drop hash part from the file name - const [_, name, ext] = href.match(/assets\/([a-z]+)\..*?\.(.*)$/) - return `${name}.${ext}` - }) + return await Promise.all( + links.map((handle) => { + return handle.evaluate((link) => ({ + pathname: new URL(link.href).pathname, + rel: link.rel, + as: link.as + })) + }) + ) } baseOptions.forEach(({ base, label }) => { @@ -60,12 +64,37 @@ baseOptions.forEach(({ base, label }) => { await page.waitForSelector('.loaded', { state: 'attached' }) expect(await getColor('.css-dynamic-import')).toBe('green') - expect(await getChunks()).toEqual([ - 'index.css', - 'dynamic.js', - 'dynamic.css', - 'static.js', - 'index.js' + expect(await getLinks()).toEqual([ + { + pathname: expect.stringMatching(/^\/assets\/index\..+\.css$/), + rel: 'stylesheet', + as: '' + }, + { + pathname: expect.stringMatching(/^\/assets\/dynamic\..+\.css$/), + rel: 'preload', + as: 'style' + }, + { + pathname: expect.stringMatching(/^\/assets\/dynamic\..+\.js$/), + rel: 'modulepreload', + as: 'script' + }, + { + pathname: expect.stringMatching(/^\/assets\/dynamic\..+\.css$/), + rel: 'stylesheet', + as: '' + }, + { + pathname: expect.stringMatching(/^\/assets\/static\..+\.js$/), + rel: 'modulepreload', + as: 'script' + }, + { + pathname: expect.stringMatching(/^\/assets\/index\..+\.js$/), + rel: 'modulepreload', + as: 'script' + } ]) }) } @@ -79,7 +108,13 @@ baseOptions.forEach(({ base, label }) => { expect(await getColor('.css-dynamic-import')).toBe('green') // in serve there is no preloading - expect(await getChunks()).toEqual([]) + expect(await getLinks()).toEqual([ + { + pathname: '/dynamic.css', + rel: 'preload', + as: 'style' + } + ]) }) } ) diff --git a/playground/css-dynamic-import/index.js b/playground/css-dynamic-import/index.js index 9115d2b4aa007e..5a0c724da737db 100644 --- a/playground/css-dynamic-import/index.js +++ b/playground/css-dynamic-import/index.js @@ -1,5 +1,10 @@ import './static.js' +const link = document.head.appendChild(document.createElement('link')) +link.rel = 'preload' +link.as = 'style' +link.href = new URL('./dynamic.css', import.meta.url).href + import('./dynamic.js').then(async ({ lazyLoad }) => { await lazyLoad() })