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): externalize network imports during ssrLoadModule #15599

Merged
merged 11 commits into from
Jan 15, 2024

Conversation

hi-ogawa
Copy link
Collaborator

@hi-ogawa hi-ogawa commented Jan 14, 2024

Description

I haven't fully tested yet, but one cause of the issue is that ssr nodeImport is trying to tryNodeResolve on external url. I'll experiment this further and adding tests etc...
(Now I added a test, but the way I did is probably overkill e.g. esm.sh dependence, experimental flag, etc... This was a easy way to verify locally, so I started with this. I would appreciate any suggestions on how it should be tested.)

Additional context

I made this repro https://github.com/hi-ogawa/repro-vite-ssr-network-imports to understand the current state. It looks like transformRequest(... { ssr: true }) is preserving network imports properly but ssrLoadModule is failing because of nodeImport/tryNodeResolve.

Network import is experimental on NodeJS, so users would need to enable --experimental-network-imports flags manually if they need this feature https://nodejs.org/api/esm.html#https-and-http-imports. (I wonder if there's any Vite-based SSR framework on Deno. if there is one, then I feel more people would be blocked by this.)

I'm not sure about the actual usefulness of network import feature myself, but just letting the external url go through during ssr dev seems reasonable as it would also align with other places such as import analysis during build:

// skip external / data uri
if (isExternalUrl(specifier) || isDataUrl(specifier)) {
continue
}

Additionally, there is an issue on Vitest vitest-dev/vitest#4949, but since vite-node doesn't use ssrLoadModule directly, I'm suspecting that the cause is different and Vite-side fix is not relevant for Vitest.


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.

Copy link

stackblitz bot commented Jan 14, 2024

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@hi-ogawa hi-ogawa changed the title fix(ssr): allow network imports feat(ssr): support network imports Jan 14, 2024
@hi-ogawa hi-ogawa marked this pull request as ready for review January 14, 2024 08:50
@omridevk
Copy link

@hi-ogawa
My use case is that I want to implement microfrontend using HTTP imports, this works fine in the browser, but I want to support SSR as well, which I will need to support HTTP import on node js as well.

@omridevk
Copy link

@hi-ogawa
Oh and thanks a lot for the PR

@omridevk
Copy link

omridevk commented Jan 14, 2024

@hi-ogawa
Maybe for testing we can setup a local static asset that just returns a js file with a variable

As we cannot use external urls that we do not have control over for testing, esm.sh may go out of business and this test will fail.

@omridevk
Copy link

@hi-ogawa also verified your fix locally and it seems to solve the error, great work :)

@hi-ogawa hi-ogawa changed the title feat(ssr): support network imports fix(ssr): externalize network imports during dev Jan 14, 2024
@hi-ogawa
Copy link
Collaborator Author

hi-ogawa commented Jan 14, 2024

@omridevk Thanks for confirming the fix on your end!

Regarding the tests, actually there was esm.sh dependency in other places in the past, but it looks like they was just removed recently due to test flakiness #14684 Thanks for the suggestion. For the purpose of the review, I think this is okay, but I'll think about avoiding external network request here.

@hi-ogawa hi-ogawa changed the title fix(ssr): externalize network imports during dev fix(ssr): externalize network imports during ssrLoadModule Jan 14, 2024
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.

Great test! And this looks like the right spot to add the check.

I know there are Vite plugins that handles URL imports, but I think they should be resolving those import paths to a safe id / virtual id, so the nodeImport never hits for those imports. So might be safe there.

@bluwy bluwy added p3-minor-bug An edge case that only affects very specific usage (priority) feat: ssr labels Jan 15, 2024
@patak-dev patak-dev enabled auto-merge (squash) January 15, 2024 16:49
@patak-dev patak-dev merged commit af2aa09 into vitejs:main Jan 15, 2024
12 checks passed
@hi-ogawa hi-ogawa deleted the fix-ssr-network-imports branch January 15, 2024 23:06
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.

running vite.ssrLoadModule causes error when using ESM CDN for example (esm.sh)
4 participants