fix: reusing variable names in html module scripts (fix #6851)

[OliverTsang]

Description

fixes #6851

I found that Pre-bundling will fail when variable names are reused between html module scripts.

This error is caused by esbuildScanPlugin in vite. This plugin handles two cases internally:

1. using variables with the same name in both normal <script> and <script setup>(Vue)
2. using variables with the same name in both normal <script> and <script context="module">(Svelte)

But it misses handling this case:

• using variables with the same name in module <script>(Html)

I think a suitable fix is treating all scripts as separated contexts during import scan phase.

Additional context

Probably need more perspective to see if this makes sense

---

What is the purpose of this pull request?

• Bug fix
• New Feature
• Documentation update
• Other

Before submitting the PR, please make sure you do the following

• Read the Contributing Guidelines.
• Read the Pull Request Guidelines and follow the Commit Convention.
• Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
• Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
• Ideally, include relevant tests that fail without this PR but pass with it.

[bluwy]

This idea makes sense to me, we should definitely have this work for HTML too. I made some comments below, but have another request here (can't highlight in the diff):

---

We can remove this variable too:

vite/packages/vite/src/node/optimizer/scan.ts

Line 236 in 7d52fe8

let loader: Loader = 'js'

and for the below usage:

vite/packages/vite/src/node/optimizer/scan.ts

Line 310 in 7d52fe8

loader,

return js directly.

[OliverTsang]

@bluwy Thanks for your review, I have made some changes based on your suggestion.

---

However, I have one more confusion about the following comment:

We can remove this variable too:

vite/packages/vite/src/node/optimizer/scan.ts

Line 236 in 7d52fe8

let loader: Loader = 'js'

and for the below usage:

vite/packages/vite/src/node/optimizer/scan.ts

Line 310 in 7d52fe8

loader,

return js directly.

It seems that we can not directly remove the loader variable, because we also use it in these two places:

vite/packages/vite/src/node/optimizer/scan.ts

Line 282 in 983feb2

(loader.startsWith('ts') ? extractImportPaths(content) : '')

vite/packages/vite/src/node/optimizer/scan.ts

Line 312 in 983feb2

contents: await transformGlob(js, path, config.root, loader)

but we can directly return loader: 'js' as you said (because the content in the js variable only contains import and export).

[bluwy]

It seems that we can not directly remove the loader variable, because we also use it in these two places:

In the PR's code, the loader is only used inside the while loop, so we can create the loader variable in the loop block. The two code blocks you linked are from the main branch, but if you map the changes to this PR, the outer-scope let loader variable can be removed.

[bluwy]

Nice. This looks good to me. I want to test this locally on a svelte project too to confirm that it still works as usual, but putting my approval first as it seems like a safe change.

[bluwy]

Ah looks like there's a case I've overlooked. If you use a named import on a svelte/vue file, it will fail with this PR. e.g.

<!-- Component.svelte -->
<script context="module">
  export const named = 'foo'
</script>

import { named } from "./Component.svelte"

The Component.svelte now doesn't contain any named exports since it's virtualized. I think we may want to handle these special cases for vue and svelte files so that it works again. Mainly Svelte's <script context="module"> and Vue's <script> should not be virtualized.

[OliverTsang]

@bluwy I think we can use export * from 'virtual module path' to solve the problem, which does not depend on a specific framework. but I'm not sure if there are other edge cases 🤔

[bluwy]

That's a pretty smart strategy. I tested it locally and it's working fine now. But I think there's a caveat though if we have two same export name from the star exports, especially for Svelte files only since exports in <script context="module"> means module exports, exports in <script> means component props. Vue is fine.

So we might still want to handle that for Svelte, otherwise the rest seems fine to me.

[OliverTsang]

I'm not familiar with Svelte, so thank you very much for this case! 👍 I made some changes, you can confirm if I understand what you mean @bluwy

[bluwy]

Nice, this LGTM. Thanks for making the changes!

[OliverTsang]

I found that this PR can solve part of the problem of another issue #6692 🤔

In the main branch, The error mentioned in the #6692 will also occur in the pre-bundle, because when the code in the js variable does not contain export default, we add export default {} at the end.

vite/packages/vite/src/node/optimizer/scan.ts

Line 304 in 33f9671

if (!path.endsWith('.vue') || !js.includes('export default')) {

vite/packages/vite/src/node/optimizer/scan.ts

Line 305 in 33f9671

js += '\nexport default {}'

This processing misses the case of export { default } from '...'. This eventually leads to getting an error like this: Multiple exports with the same name "default"

With this PR, we can avoid this error when running pre-bundling, because we virtualized all scripts in Vue.

---

But we still get the same error in plugin-vue, because in this case, the code generated by vue/compiler-sfc will contain two default exports.

[bluwy]

Nice find, yeah it looks like this PR partially fixes #6692 too (the scan part). The build part may be an issue in @vitejs/plugin-vue itself though instead of @vue/compiler-sfc since there's these lines.

[patak-dev]

Thanks for the PR @OliverTsang, added it to be discussed in our next team meeting. We should merge this one in the 2.9 beta if we proceed so we can get some extra testing from Vue and Svelte folks

[patak-dev]

We talked about this PR and decided to move forward with it. We are going to merge it once the Vite 2.9 beta starts to be able to properly test it.