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(ssr): skip esm proxy guard for namespace imports #14988

Merged
merged 1 commit into from
Nov 15, 2023

Conversation

bluwy
Copy link
Member

@bluwy bluwy commented Nov 15, 2023

Description

repro https://github.com/brillout/vite-regression-2023-nov-14

We add the ESM proxy guard unconditionally to error when you import something like:

import { non_existent } from './mod'

However, we don't have to add the guard for

import * as mod from './mod'

if (mod.non_existent) // ...

This PR fixes this by checking if they are imported names from the ./mod before adding the guard. In which case, namespace imports would never have imported names.


Other things I changed along the way:

  1. Renamed namedImportSpecifiers to importedNames: Matches the AST name and because the array can now include "default" when you do import mod from './mod'. In reality, this is the same as import { default as mod } from './mod' and I noticed Node.js handles this way too.
  2. For the emulated Node.js errors, add a [vite] prefix so we can easily identify if it's coming from Vite in the future.

Additional context


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, especially the Pull Request Guidelines.
  • 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).
  • Update the corresponding documentation if needed.
  • Ideally, include relevant tests that fail without this PR but pass with it.

@bluwy bluwy added p3-downstream-blocker Blocking the downstream ecosystem to work properly (priority) feat: ssr labels Nov 15, 2023
Copy link
Member

@sapphi-red sapphi-red left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Maybe we can have a test in packages/vite/src/node/ssr/__tests__/ssrLoadModule.spec.ts checking the three types.

import 'foo'
import * as foo from 'foo'
import { foo } from 'foo'

@patak-dev
Copy link
Member

/ecosystem-ci run vike

@bluwy
Copy link
Member Author

bluwy commented Nov 15, 2023

Maybe we can have a test in packages/vite/src/node/ssr/__tests__/ssrLoadModule.spec.ts checking the three types.

Yeah I think that would be nice. There's probably a lot of node.js compat tests we can put there now that you mention it. I'll work on this in a following PR 👍

@vite-ecosystem-ci
Copy link

📝 Ran ecosystem CI on 0eddb1f: Open

suite result latest scheduled
vike failure failure

@patak-dev
Copy link
Member

/ecosystem-ci run

@vite-ecosystem-ci
Copy link

📝 Ran ecosystem CI on 0eddb1f: Open

suite result latest scheduled
analogjs success success
astro success success
histoire failure failure
ladle success success
laravel failure failure
marko success success
nuxt success failure
nx failure failure
previewjs success success
qwik failure failure
rakkas success success
sveltekit failure failure
unocss success success
vike failure failure
vite-plugin-pwa success success
vite-plugin-react success success
vite-plugin-react-pages success success
vite-plugin-react-swc success success
vite-plugin-svelte success success
vite-plugin-vue success success
vite-setup-catalogue success success
vitepress success success
vitest failure failure

@patak-dev patak-dev merged commit 82a5b11 into main Nov 15, 2023
10 checks passed
@patak-dev patak-dev deleted the fix-ssr-import-false-checks branch November 15, 2023 09:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: ssr p3-downstream-blocker Blocking the downstream ecosystem to work properly (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants