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

fix: don't duplicate styles with dynamic import (fix #9967) #9970

Merged
merged 4 commits into from
Sep 23, 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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 18 additions & 2 deletions packages/vite/src/node/plugins/importAnalysisBuild.ts
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,8 @@ function preload(
return baseModule()
}

const links = document.getElementsByTagName('link')

return Promise.all(
deps.map((dep) => {
// @ts-ignore
Expand All @@ -75,10 +77,24 @@ function preload(
seen[dep] = true
Copy link
Member

Choose a reason for hiding this comment

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

This is just an idea but maybe we could simplify the logic if we use new URL(dep, document.baseURI).href. If the SSR markup used an absolute path, I guess the current approach won't work. (it didn't work from before though)

Copy link
Contributor

Choose a reason for hiding this comment

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

that was what I missed! To supply the base url!
Earlier I used new URL(dep).href that didn't make sense when dep is relative, just understand it now, according to MDN about the constructor signature new URL(url, base):

If url is a relative URL, base is required, and will be used as the base URL. If url is an absolute URL, a given base will be ignored.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is just an idea but maybe we could simplify the logic if we use new URL(dep, document.baseURI).href. If the SSR markup used an absolute path, I guess the current approach won't work. (it didn't work from before though)

NOOOOOOO! BaseURI isn't supported on IE11!

Copy link
Contributor

Choose a reason for hiding this comment

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

This is just an idea but maybe we could simplify the logic if we use new URL(dep, document.baseURI).href. If the SSR markup used an absolute path, I guess the current approach won't work. (it didn't work from before though)

NOOOOOOO! BaseURI isn't supported on IE11!

OK, since this PR doesn't introduce legacy support, I think it's fine to use BaseURI.
It seems that a correct place to handle this issue is at my PR #9920, since right now the preload function isn't called for legacy browsers.
Therefore, @bgoscinski, you may ignore the rest of this comment, I'll handle with it after in my PR.

The full solution was introduced in sapper legacy support, on PR sveltejs/sapper#1562.
I'll explain briefly the logic of this sapper PR, how it handle the situation when document.baseURI isn't available:
We could have used the document.URL which is supported by IE11. Sadly, due to the reasons in this stackoverflow answer, if there is some <base> element in the DOM, it changes the base path, ignoring the current URL(demo is included in the stackoverflow answer). Luckily, by the specifications, there could be only one at most such <base> element, so we just need to grab the one we see, and take this to be the baseURI. If no such tag were found, we can simply take document.URL instead.

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) {
// When isBaseRelative is true then we have `importerUrl` and `dep` is
// already converted to an absolute URL by the `assetsURL` function
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
}
Comment on lines +80 to 96
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe @sapphi-red suggestion here was to do my initial thought, just in a way that actually fix my issue?
As far as I can tell, it omits the if (isBaseRelative), and the suggestion is to do this:

Suggested change
const isBaseRelative = !!importerUrl
// check if the file is already preloaded by SSR markup
if (isBaseRelative) {
// When isBaseRelative is true then we have `importerUrl` and `dep` is
// already converted to an absolute URL by the `assetsURL` function
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
}
const absoluteDep = new URL(dep, document.baseURI).href;
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 === absoluteDep && (!isCss || link.rel === 'stylesheet')) {
return
}
}


// @ts-ignore
const link = document.createElement('link')
// @ts-ignore
Expand Down
121 changes: 121 additions & 0 deletions playground/css-dynamic-import/__tests__/css-dynamic-import.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,121 @@
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'] },
build: { assetsInlineLimit: 0 }
})

async function withBuild(base: string, fn: () => Promise<void>) {
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<void>) {
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 getLinks() {
const links = await page.$$('link')
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 }) => {
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 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'
}
])
})
}
)

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 getLinks()).toEqual([
{
pathname: '/dynamic.css',
rel: 'preload',
as: 'style'
}
])
})
}
)
})
10 changes: 10 additions & 0 deletions playground/css-dynamic-import/__tests__/serve.ts
Original file line number Diff line number Diff line change
@@ -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()
}
}
3 changes: 3 additions & 0 deletions playground/css-dynamic-import/dynamic.css
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
.css-dynamic-import {
color: green;
}
6 changes: 6 additions & 0 deletions playground/css-dynamic-import/dynamic.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
import './dynamic.css'

export const lazyLoad = async () => {
await import('./static.js')
document.body.classList.add('loaded')
}
3 changes: 3 additions & 0 deletions playground/css-dynamic-import/index.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
<p class="css-dynamic-import">This should be green</p>

<script type="module" src="./index.js"></script>
10 changes: 10 additions & 0 deletions playground/css-dynamic-import/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +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()
})
3 changes: 3 additions & 0 deletions playground/css-dynamic-import/static.css
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
.css-dynamic-import {
color: red;
}
3 changes: 3 additions & 0 deletions playground/css-dynamic-import/static.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
import './static.css'

export const foo = 'foo'
3 changes: 2 additions & 1 deletion playground/test-utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down