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

Non-modern build should also ("pre")load CSS #9902

Open
4 tasks done
Tal500 opened this issue Aug 29, 2022 · 12 comments · May be fixed by #9920
Open
4 tasks done

Non-modern build should also ("pre")load CSS #9902

Tal500 opened this issue Aug 29, 2022 · 12 comments · May be fixed by #9920

Comments

@Tal500
Copy link
Contributor

Tal500 commented Aug 29, 2022

Description

While I'm doing my efforts in sveltejs/kit#6265 to add legacy support to SvelteKit on the Vite way, I had noticed that when navigating from one page to the other, the internal function __vitePreload is called, which is defined in the plugin importAnalysisBuild.

As far as I understand, the purpose of this function is to preload JS modules and also to (pre?)load CSS files.
On legacy output chunks, it shuts down the preloading ability.
I understand that there is no need to preload JS modules on non-modern(i.e. non ESModule compatible) browsers, but for the correct way of loading CSS, the CSS files should(?) be loaded in __vitePreload.

According to my local tests, if the CSS are getting preloaded on legacy builds, everything works great in my legacy work on SvelteKit.

Suggested solution

Change the code of the plugin importAnalysisBuild to (pre?)load CSS anyway, and when the browser is also modern, import also the preloaded JS files.

Alternative

You tell me?

Additional context

No response

Validations

@Tal500 Tal500 changed the title non-modern build should also ("pre")load CSS Non-modern build should also ("pre")load CSS Aug 29, 2022
@fejiroofficial
Copy link

Hello Tal500. I appreciate your effort put in here. I was trying out your solution on our code, but the production build was failing due to this: Is this issue in any way related?
[vite-plugin-svelte-kit] Prerendering failed with code 1 error during build: Error: Prerendering failed with code 1 at ChildProcess.<anonymous> (file:///Users/fejirogospel/Documents/GitHub/center-nx/apps/site-app/node_modules/@sveltejs/kit/src/exports/vite/index.js:455:14) at ChildProcess.emit (node:events:527:28) at Process.ChildProcess._handle.onexit (node:internal/child_process:291:12)

@Tal500
Copy link
Contributor Author

Tal500 commented Aug 29, 2022

Hello Tal500. I appreciate your effort put in here. I was trying out your solution on our code, but the production build was failing due to this: Is this issue in any way related? [vite-plugin-svelte-kit] Prerendering failed with code 1 error during build: Error: Prerendering failed with code 1 at ChildProcess.<anonymous> (file:///Users/fejirogospel/Documents/GitHub/center-nx/apps/site-app/node_modules/@sveltejs/kit/src/exports/vite/index.js:455:14) at ChildProcess.emit (node:events:527:28) at Process.ChildProcess._handle.onexit (node:internal/child_process:291:12)

Hello! Are you talking about the SvelteKit legacy PR I made? If so, you better comment this issue on the PR I sent on SvelteKit, not here.(was it a mistake?)

Anyway, for your case, can you:

  1. Disable prerendering (in svelte.config.js), do npm run build && npm run preview, and share the result? Also, can you please publish a bigger chunk of the console error? It's seems that a detailed error message appears earlier.
  2. Can you give a sample code for reproduction?
  3. Did you try to compile and run the simple demo?

Please comment to me to this on the PR in SvelteKit, for a better discussion.

@Tal500 Tal500 linked a pull request Aug 30, 2022 that will close this issue
8 tasks
@Tal500
Copy link
Contributor Author

Tal500 commented Aug 30, 2022

Update: After working on PR #9920, I've changed my opinion - I think that on legacy builds we should have full preloading support.

@Tal500
Copy link
Contributor Author

Tal500 commented Sep 3, 2022

After some investigation, I realized that Vite always inline the legacy CSS in injected HTML code. But why?

@yyx990803 , what is the reason for 940d483 ? Wouldn't it be nicer to also load the CSS normally, the same way you load it in modern browsers? (any technical difference between modern and legacy here in CSS?)

@sapphi-red
Copy link
Member

It's described here. #2062 (comment)

@Tal500
Copy link
Contributor Author

Tal500 commented Sep 4, 2022

It's described here. #2062 (comment)

Thank you for the reference!

So as far as I see, there is no principle from Vite maintainers for not preloading content on legacy, right?
This can easily solves all the problem, and will allow us to get rid of inlining css issues on legacy🙂

My PR #9920 does exactly this. If we want preloading on legacy, we can get rid of all the CSS inlining mess (update: this PR got rid of it).

Did I get it right? Do you agree we shall do it?

@sapphi-red
Copy link
Member

IIUC Vite inlines CSS to realize preloading CSS by using JS chunks instead.
The reason why it isn't using the __vitePreload seems to be because linkElement.onload does not work in legacy browsers.

webpack-contrib/mini-css-extract-plugin#294
https://pie.gd/test/script-link-events/

@Tal500
Copy link
Contributor Author

Tal500 commented Sep 4, 2022

IIUC Vite inlines CSS to realize preloading CSS by using JS chunks instead. The reason why it isn't using the __vitePreload seems to be because linkElement.onload does not work in legacy browsers.

webpack-contrib/mini-css-extract-plugin#294 https://pie.gd/test/script-link-events/

OK, I read the sources, so now I understand the "XHR request" meaning on #2062 (comment) .

So I will simply make such a request on legacy, and it will just "embed" the content of the CSS file between <style>...</style> tags.
There will be an attribute data-href={original_source} on the style tag for future detection, to prevent loading the same stylesheet twice.

Sounds good?

@sapphi-red
Copy link
Member

sapphi-red commented Sep 4, 2022

I think that works for most cases but it won't for a CSS file containing relative paths (background: url('./foo.png')).
The relative paths are relative to CSS file, but when injected into <style> tag, it will be relative to location.href.

I'm not sure but an approach like https://guybedford.com/es-module-preloading-integrity might work.

@Tal500
Copy link
Contributor Author

Tal500 commented Sep 4, 2022

I think that works for most cases but it won't for a CSS file containing relative paths (background: url('./foo.png')). The relative paths are relative to CSS file, but when injected into <style> tag, it will be relative to location.href.

I'm not sure but an approach like https://guybedford.com/es-module-preloading-integrity might work.

How about do (in parallel):

  1. Inject the link tag after
  2. Query XHR to fetch(and cache) and see if the resource available, see also errors we're getting.

Do you think that the browser will execute the query twice? It seems that the caching should be immediate, but I'm not sure.
It's kinda like the loading to image tag trick and then on error we know that the resource fetching has been finished.

Or to calculate correctly how to get the correct path.

We can also "give up" on the detection of error in CSS path on legacy, and then use the trick of loading to image and then to link.

@Tal500
Copy link
Contributor Author

Tal500 commented Sep 4, 2022

I think that works for most cases but it won't for a CSS file containing relative paths (background: url('./foo.png')). The relative paths are relative to CSS file, but when injected into <style> tag, it will be relative to location.href.

I'm not sure but an approach like https://guybedford.com/es-module-preloading-integrity might work.

I had updated PR #9920 to do this things.
Tell me what do you think about the new preload function:

function preload(
baseModule: () => Promise<{}>,
deps?: string[],
importerUrl?: string
) {
if (!deps || deps.length === 0) {
return baseModule()
}
return Promise.all(
deps.map((dep) => {
// @ts-ignore
dep = assetsURL(dep, importerUrl)
// @ts-ignore
if (dep in seen) return
// @ts-ignore
seen[dep] = true
const seperatorIdx = dep.lastIndexOf('/')
const shortDep = seperatorIdx >= 0 ? dep.slice(seperatorIdx + 1) : dep
const isCss = dep.endsWith('.css')
const cssSelector = isCss ? '[rel="stylesheet"]' : ''
// @ts-ignore check if the file is already preloaded by SSR markup
const possibleLinks = document.querySelectorAll<HTMLLinkElement>(
`link[href$="${shortDep}"]${cssSelector}`
)
for (let i = 0; i < possibleLinks.length; ++i) {
const currentPath = possibleLinks[i].href
if (
currentPath === dep ||
new URL(currentPath, importerUrl).href === dep
) {
return
}
}
const preloadInImg = (res: () => void) => {
const img = new Image()
img.onerror = () => res()
img.onload = () => res()
img.src = dep
}
const loadLink = () => {
// @ts-ignore
const link = document.createElement('link')
// @ts-ignore
link.rel = isCss ? 'stylesheet' : scriptRel
if (!isCss) {
link.as = 'script'
link.crossOrigin = ''
}
link.href = dep
// @ts-ignore
document.head.appendChild(link)
return link
}
const waitForLoad = (link: HTMLLinkElement) => {
// @ts-ignore
if (__VITE_IS_MODERN__) {
return new Promise<void>((res, rej) => {
link.addEventListener('load', () => res())
link.addEventListener('error', () =>
rej(new Error(`Unable to preload CSS for ${dep}`))
)
})
} else {
return Promise.all<void>([
new Promise((res, rej) => {
// We query the path (that should be cached already), for knowing if there is an error while downloading it. (this is the only reason)
const req = new XMLHttpRequest()
req.addEventListener('load', () => res())
req.addEventListener('error', () =>
rej(
new Error(
`Unable to preload CSS (via XMLHttpRequest) for ${dep}`
)
)
)
req.open('GET', dep)
req.send()
}),
new Promise((res) => {
// On legacy browsers, let them a chance to process the newly referred CSS link.
setTimeout(res)
})
]) as unknown as Promise<void>
}
}
const loadLinkAndWait = () => {
return waitForLoad(loadLink())
}
// @ts-ignore
if (__VITE_IS_MODERN__) {
if (isCss) {
return loadLinkAndWait()
} else {
loadLink()
}
} else {
if (isCss) {
return new Promise<void>(preloadInImg).then(loadLinkAndWait)
} else {
preloadInImg(() => {})
loadLink()
}
}
})
).then(() => baseModule())
}

@mreduar
Copy link

mreduar commented Aug 8, 2023

Any update here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants