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): failed ssrLoadModule call throws same error #7177

Merged
merged 3 commits into from Apr 30, 2022

Conversation

TrickyPi
Copy link
Contributor

@TrickyPi TrickyPi commented Mar 5, 2022

Description

fixes #7030

Additional context

Is the new test file in the correct location?


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.

@Niputi Niputi added feat: ssr p3-minor-bug An edge case that only affects very specific usage (priority) labels Mar 5, 2022
Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

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

The fix looks good to me, but I think the tests should be in packages/vite/src/node/ssr/__tests__/ssrModuleLoader.spec.ts to group the SSR tests.

@bluwy bluwy changed the title fix(vite): always throw error when evaluating an wrong SSR module fix(ssr): always throw error when evaluating wrong SSR module Mar 5, 2022
@TrickyPi
Copy link
Contributor Author

TrickyPi commented Mar 5, 2022

okay, done.

bluwy
bluwy previously approved these changes Mar 5, 2022
@Rich-Harris
Copy link
Contributor

This is super nitpicky but this PR isn't quite in line with the behaviour of Node and browsers. If a module throws an error during instantiation, subsequent imports will throw the same error object:

let e1, e2;

try {
  await import('./bad.js');
} catch (e) {
  e1 = e;
}

try {
  await import('./bad.js');
} catch (e) {
  e2 = e;
}

assert.equal(e1, e2);

With this PR, the module is re-instantiated, and a new error is thrown. I was working on a PR separately, unaware this had already been submitted, and the solution I landed on was this — basically, store the error as mod.ssrError, and throw it in instantiateModule if it exists.

@patak-dev patak-dev changed the title fix(ssr): always throw error when evaluating wrong SSR module fix(ssr): failed ssrLoadModule call throws same error Apr 30, 2022
@patak-dev patak-dev merged commit 891e7fc into vitejs:main Apr 30, 2022
@TrickyPi TrickyPi deleted the fix-7030 branch May 1, 2022 03:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: ssr p3-minor-bug An edge case that only affects very specific usage (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ssrLoadModule succeeds when it should fail if called a second time
5 participants