fix: avoid scan failures in .svelte and .astro files

[benmccann]

Description

The dev server would crash with a hard-to-diagnose error if export default was contained anywhere in a .svelte file

The code comment explains the fix pretty well

Additional context

I hit this while working on conversion of svelte.dev to SvelteKit. Thanks to @bluwy for tracking it down!

This is pretty easy to hit in SvelteKit. The following line will do it:

const dev = process.env.NODE_ENV === 'development';

process.env.NODE_ENV gets replace with "development" causing a failure because there's a sourcemap embedded in the template the double quotes aren't escaped causing an invalid syntax.

I imagine this could easily be triggered in other template types as well though perhaps not quite as often and with less severe effects. E.g. if you said:

<div>
  <p>
    This is my blog post talking about how to use process.env.NODE_ENV.
    Except you won't see that text. You'll see: This is my blog post talking about how to use "development"
  </p>
<div>

That will probably lead to unexpected results in any templating language

---

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.

[patak-dev]

Could this be added to vite-plugin-svelte? Maybe as an esbuild plugin? We should try to avoid adding more conditional depending on a framework. If not, maybe a regex could be added that doesn't have this false positive? (ping @dominikg)

[benmccann]

I agree it's not the best long-term fix to add checks for a specific template type. I'm afraid that trying to put it in vite-plugin-svelte would just hide this error though, which is likely to affect others as well beside just Svelte. The Astro folks have confirmed this affects them as well, so I've updated it to include .astro as well. It seems like longer-term we need some way for the plugins to pass options to Vite, so that Astro and Svelte can pass something like canHaveDefaultExport: false

I'm not sure it's really possible to catch these cases with a better regex for the most part. The only way to really do this reliably is to parse the page

[Shinigami92]

In that case, I would suggest directly to implement such an option instead of patch it that way

[benmccann]

Is there an example of a similar option I could follow? I'm not quite sure how to make the change and have only seen checks for the file extension elsewhere

[bluwy]

I think a patch is the better choice here, either way an option would feel hack-ish as well. So having some form of fix before the anticipated improved dependency handling is fine IMO.

Also it looks like the "additional context" is a bit "out of context" (maybe). I think it is that we had a template string of export default that tripped up Vite's scanner, because we actually want Vite to still shim the export default {} code. The process.env.NODE_ENV issue is because Vite does code replacement in the post-phase, which caused sveltejs/kit#720.

[dominikg]

The reason there is a naive string check for export default is that script blocks in .vue files and possibly others? can legally contain a default export and injecting another one would lead to duplicate export default, tripping it up.

So we are looking for a condition that tells us if we have to do that naive check or if it is safe to unconditionally inject the export.

An additional option in vite config that can be set by plugins sounds better than hardcoding it, but @bluwy is right. If this is just a stop-gap measure and the option will become useless soon, patching it in place seems less invasive.

The question then becomes if we want to use inclusion or exclusion

const TYPES_WITH_DEFAULT_EXPORT =['.vue',/*...*/];
if(!TYPES_WITH_DEFAULT_EXPORT.some(t => path.endsWith(t)) {
...
}

const TYPES_WITHOUT_DEFAULT_EXPORT =['.svelte',/*...*/];
if(TYPES_WITHOUT_DEFAULT_EXPORT.some(t => path.endsWith(t)) {
...
}

cc @drwpow for .astro, it may need additional changes (there are more hardcoded checks for .svelte and .vue in this file)

[yyx990803]

I think for now the logic can be refactored to:

Only skip appending export default if:

• The file is a .vue file
• The extracted js already contains export default

This would technically still have a slight chance of false positive if the js code contains strings with export default, but I'll work around that later.

[benmccann]

Updated per the suggestions. Thanks for the review!

requested changes done