refactor(build): remove quotes from preload marker

[PengBoUESTC]

Description

relaxing preloadMarkerWithQuote reg to replace str like this :

"__VITE_PRELOA\
D\
__"

But , there will be some performance problems.

#16529

[stackblitz]

Run & review this pull request in StackBlitz Codeflow.

[bluwy]

Hmm, you're right. I didn't think that esbuild could break the \ at any position. I think some performance impact is expected, but unsure by how much.

I wonder if it's better to skip strings altogether though and use a plain __VITE_PRELOAD__ global? Tooling shouldn't mangle that and it should always stay as is. We can skip the quotes regex too? 🤔

[PengBoUESTC]

Hmm, you're right. I didn't think that esbuild could break the \ at any position. I think some performance impact is expected, but unsure by how much.

I wonder if it's better to skip strings altogether though and use a plain __VITE_PRELOAD__ global? Tooling shouldn't mangle that and it should always stay as is. We can skip the quotes regex too? 🤔

Maybe a plain __VITE_PRELOAD__ is better, so that esbuild will not break this string, and we can remove the quotes in preloadMarkerWithQuote

[bluwy]

/ecosystem-ci run

Checking the ecosystem for this to see if it works. If it does, I think we can further improve the code since we're no longer dealing with quotes.

[vite-ecosystem-ci]

📝 Ran ecosystem CI on 0f0a1cf: Open

suite	result	latest scheduled
vitest	❌ failure	❌ failure

✅ analogjs, astro, histoire, ladle, laravel, marko, nuxt, previewjs, quasar, qwik, rakkas, remix, sveltekit, unocss, vike, vite-plugin-pwa, vite-plugin-react, vite-plugin-react-pages, vite-plugin-react-swc, vite-plugin-svelte, vite-plugin-vue, vite-setup-catalogue, vitepress

[bluwy]

Seems like this strategy works fine too. I think we can polish up the code a bit to no longer refer it as quotes.

[PengBoUESTC]

Seems like this strategy works fine too. I think we can polish up the code a bit to no longer refer it as quotes.

replace preloadMarkerWithQuote with preloadMarkerRE

[bluwy]

Thanks! This LGTM. I'll let others have a review on this too in case I miss something.

[patak-dev]

I think the idea behind the quotes was originally to keep the code as a valid module. Maybe we should use import.meta.__VITE_PRELOAD__?

[PengBoUESTC]

I think the idea behind the quotes was originally to keep the code as a valid module. Maybe we should use import.meta.__VITE_PRELOAD__?

Do you mean that , an undefined variable __VITE_PRELOAD__ may break the code in runtime ?

[patak-dev]

It may break if there is a plugin that is checking that all variables are properly defined. I don't know if we are respecting this in all our replacements though, just trying to understand why we have been using quotes so far. I like the idea that our plugins are generating a valid module at each step.

[bluwy]

Isn't using plain __VITE_PRELOAD__ a valid module too? It's referring to a global __VITE_PRELOAD__ variable. We also have a fallback that replaces any existing __VITE_PRELOAD__, so it should not exist in the final chunk. If it would today, I think it breaks too because preload() does not handle a "__VITE_PRELOAD__" passed in today.

I suppose the only risk here is that __VITE_PRELOAD__ is less unique than "__VITE_PRELOAD__", so it can be accidentally replaced in user code, but I think it's worth the tradeoff and is equally risky as the existing __VITE_ASSET__* constant.

EDIT: Using import.meta. __VITE_PRELOAD__, esbuild can break them up multiple lines too and wouldn't solve the issue.

[PengBoUESTC]

It may break if there is a plugin that is checking that all variables are properly defined. I don't know if we are respecting this in all our replacements though, just trying to understand why we have been using quotes so far. I like the idea that our plugins are generating a valid module at each step.

I think that, import.meta.__VITE_PRELOAD__ may have some side effect, but I want to try to check it with 'ecosystem-ci'

[PengBoUESTC]

Isn't using plain __VITE_PRELOAD__ a valid module too? It's referring to a global __VITE_PRELOAD__ variable. We also have a fallback that replaces any existing __VITE_PRELOAD__, so it should not exist in the final chunk. If it would today, I think it breaks too because preload() does not handle a "__VITE_PRELOAD__" passed in today.

I suppose the only risk here is that __VITE_PRELOAD__ is less unique than "__VITE_PRELOAD__", so it can be accidentally replaced in user code, but I think it's worth the tradeoff and is equally risky as the existing __VITE_ASSET__* constant.

EDIT: Using import.meta. __VITE_PRELOAD__, esbuild can break them up multiple lines too and wouldn't solve the issue.

U R correct, esbuild will break import.meta. __VITE_PRELOAD__ too , so , maybe we. don't need to try =_=

[patak-dev]

Sorry for the noise, ya, it looks safe to directly use an undefined variable here. I'm a bit puzzled as to why the quotes were added then. I'm good with doing this change 👍🏼

[bluwy]

I'll queue this for the next minor then to be safe 👍