feat(ssr): support for ssr.resolve.conditions and ssr.resolve.externalConditions options #14498

marbemac · 2023-09-28T20:43:46Z

Description

Currently the vite dev server (and thus ssrLoadModule) do not respect resolve.conditions, and target node even when ssr.target is webworker.

This PR introduces a new ssr.resolve option to configure how internal and external imports are resolved during SSR.

I attempted to follow the recommended behavior described in the comments of #13487.

I didn't update any documentation - can do that if there's a good chance this change will be accepted.

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 PR Title 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.

Signed-off-by: Marc MacLeod <847542+marbemac@users.noreply.github.com>

marbemac · 2023-09-29T20:00:57Z

packages/vite/src/node/plugins/resolve.ts

+      const ssrConditions =
+        resolveOptions.ssrConfig?.resolve?.conditions ||
+        resolveOptions.conditions


Maintains current behavior if the new ssr.resolve.conditions field is not set - backwards compatible.

packages/vite/src/node/ssr/ssrExternal.ts

sapphi-red

Great! Thank you!

You can ignore the CI failure on Windows. It's caused by a bug in Node.

packages/vite/src/node/ssr/ssrExternal.ts

sapphi-red · 2023-09-30T05:04:02Z

packages/vite/src/node/ssr/ssrModuleLoader.ts

+      targetWeb,
+      undefined,
+      true,


Suggested change

targetWeb,

undefined,

true,

false,

undefined,

true,

Because this part imitates Node and Node's resolver works like targetWeb=false, this needs to be false.
If we run this part with true, the same code won't run after build because of that.

Hmm, but afaik a main use case for ssr.target = 'webworker' is for when the end user is planning to run in non-node runtimes (cloudflare, deno, bun, etc). This is how I've been using it at least - for example when using ssrLoadModule in dev to load a file that imports renderToReadableStream from react-dom.

Although I suppose the end user can now achieve this same result by explicitly listing the relevant conditions in ssr.resolve - the inconsistency between how dev server pipeline and build pipeline use ssr.target just felt a little weird.

Since there's another way to achieve the desired result via the new ssr.resolve options, I can change it back to false, just please confirm with a 👍 when you have a sec.

Hmm, but afaik a main use case for ssr.target = 'webworker' is for when the end user is planning to run in non-node runtimes (cloudflare, deno, bun, etc).

If a user is going to create a bundle for non-node runtimes, we expect them to set ssr.noExternal: true.
https://vitejs.dev/guide/ssr.html#ssr-bundle
This is because it doesn't make sense to externalize any modules in that case. (Bare import specifiers won't work in those environments because those environments doesn't support node_modules resolutions.)
If the module is not externalized, ssr.target = 'webworker' would set the targetWeb for vite:resolve plugin and that would resolve the import instead of ssrLoadModule.

Makes sense, but I still think there are some rough edges when trying to use vite createServer + ssrLoadModule to create a nice local dev server experience (in conjunction w actually running dev server in non node runtimes like bun, or when wanting to better represent the final build target during local dev when that build target is non-node like cloudflare). BUT I think best as a separate discussion, the adjustments in this PR make it possible to cover everything :).

I reverted the targetWeb change - thanks for the review!

playground/ssr-conditions/src/direct-load.js

Signed-off-by: Marc MacLeod <847542+marbemac@users.noreply.github.com>

marbemac · 2023-10-02T15:02:50Z

@sapphi-red should I add some notes to docs/guide/ssr.md outlining the new ssr.resolve options, or are you/somebody else going to tackle the docs later?

sapphi-red · 2023-10-03T10:44:44Z

It would be nice if you add it. Also in /docs/config/ssr-options.md. But I can add it later too.

bluwy

Really nice tests! Adding the docs would be a plus too.

Signed-off-by: Marc MacLeod <847542+marbemac@users.noreply.github.com>

marbemac · 2023-10-03T15:46:13Z

@sapphi-red @bluwy alrighty, I took a crack at some docs. There were two styles of links in the docs (relative file path .md style links, and what look to be absolute URLs) - I chose relative file paths. Let me know if I missed anything!

Signed-off-by: Marc MacLeod <847542+marbemac@users.noreply.github.com>

sapphi-red

💚

docs/guide/ssr.md

hyf0 · 2023-10-08T04:37:24Z

Just want to ask when will this feature be released? This seems to be the key blocker for framework related to react server component.

bluwy · 2023-10-09T04:21:07Z

We don't usually backport features to v4, so this will come in v5. I can cut another beta for it today, but for stable that will come sometime between mid-late October.

Tobbe · 2023-10-17T04:45:54Z

Redwood (and I) would love it if you could make an exception and back-port this to v4 🙏
We're not ready to upgrade to Vite v5 because we're not ESM-only ready just yet (working on it though!)

…lConditions options (#14498)

bluwy · 2023-10-17T09:08:19Z

@Tobbe you don't need to be ESM-only to use Vite 5, unless you're using the ssr.format option? Using the CJS node API will still work albeit with the warning message that can be suppressed if really needed.

Tobbe · 2023-10-17T10:09:16Z

@bluwy we use legacy.buildSsrCjsExternalHeuristics: true

…nditions (#14498) (#14668) Co-authored-by: Marc MacLeod <marbemac@gmail.com>

patak-dev · 2023-10-18T09:20:42Z

@Tobbe @nksaraf @hyf0, we discussed with @bluwy and backported this PR (together with a few other minor features) and released vite 4.5.0 with it.

fix(ssr): respect conditions when target webworker

96a4c55

Signed-off-by: Marc MacLeod <847542+marbemac@users.noreply.github.com>

marbemac mentioned this pull request Sep 29, 2023

resolve conditions not respected when running the dev server / ssrLoadModule #14496

Closed

7 tasks

marbemac added 2 commits September 29, 2023 13:50

feat: add ssr conditions and externalConditions

20e890e

Signed-off-by: Marc MacLeod <847542+marbemac@users.noreply.github.com>

add some tests

399d824

Signed-off-by: Marc MacLeod <847542+marbemac@users.noreply.github.com>

marbemac changed the title ~~fix(ssr): respect resolve.conditions when ssr.target is webworker~~ feat(ssr): support for ssr.resolve.conditions and ssr.resolve.externalConditions options Sep 29, 2023

marbemac commented Sep 29, 2023

View reviewed changes

sapphi-red added p3-downstream-blocker Blocking the downstream ecosystem to work properly (priority) feat: ssr labels Sep 30, 2023

sapphi-red reviewed Sep 30, 2023

View reviewed changes

marbemac added 2 commits September 30, 2023 12:11

pr feedback

7b225ec

Signed-off-by: Marc MacLeod <847542+marbemac@users.noreply.github.com>

pr feedback

3fc8cc1

Signed-off-by: Marc MacLeod <847542+marbemac@users.noreply.github.com>

sapphi-red previously approved these changes Oct 3, 2023

View reviewed changes

patak-dev previously approved these changes Oct 3, 2023

View reviewed changes

bluwy previously approved these changes Oct 3, 2023

View reviewed changes

docs

72c0e5c

Signed-off-by: Marc MacLeod <847542+marbemac@users.noreply.github.com>

marbemac dismissed stale reviews from bluwy, patak-dev, and sapphi-red via 72c0e5c October 3, 2023 15:45

tiny docs

e6b20fb

Signed-off-by: Marc MacLeod <847542+marbemac@users.noreply.github.com>

sapphi-red previously approved these changes Oct 4, 2023

View reviewed changes

bluwy reviewed Oct 4, 2023

View reviewed changes

docs/guide/ssr.md Outdated Show resolved Hide resolved

bluwy dismissed sapphi-red’s stale review via 4b4912c October 4, 2023 08:44

docs: capitalize subheading

4b4912c

bluwy approved these changes Oct 4, 2023

View reviewed changes

bluwy merged commit d0afc39 into vitejs:main Oct 5, 2023

marbemac mentioned this pull request Oct 5, 2023

fix: respect resolve.conditions during nodeResolve #13487

Closed

9 tasks

bluwy mentioned this pull request Oct 6, 2023

fix: respect resolve.conditions during nodeResolve #14417

Closed

dai-shi mentioned this pull request Oct 7, 2023

Patch Vite and update dependencies dai-shi/waku#136

Merged

2 tasks

patak-dev pushed a commit that referenced this pull request Oct 17, 2023

feat(ssr): support for ssr.resolve.conditions and ssr.resolve.externa…

6240f83

…lConditions options (#14498)

patak-dev mentioned this pull request Oct 17, 2023

feat(ssr): backport ssr.resolve.conditions and ssr.resolve.externalConditions (#14498) #14668

Merged

4 tasks

patak-dev added a commit that referenced this pull request Oct 18, 2023

feat(ssr): backport ssr.resolve.conditions and ssr.resolve.externalCo…

520139c

…nditions (#14498) (#14668) Co-authored-by: Marc MacLeod <marbemac@gmail.com>

Tobbe mentioned this pull request Oct 22, 2023

Upgrade to Vite v4.5.0 redwoodjs/redwood#9328

Merged

himself65 mentioned this pull request Oct 23, 2023

feat: upgrade to vite 4.5.0 dai-shi/waku#141

Merged

hi-ogawa mentioned this pull request May 10, 2024

fix(ssr): exclude module condition from ssr node resolve #16647

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ssr): support for ssr.resolve.conditions and ssr.resolve.externalConditions options #14498

feat(ssr): support for ssr.resolve.conditions and ssr.resolve.externalConditions options #14498

marbemac commented Sep 28, 2023 •

edited

Loading

marbemac Sep 29, 2023

sapphi-red left a comment

sapphi-red Sep 30, 2023

marbemac Sep 30, 2023

sapphi-red Oct 1, 2023

marbemac Oct 2, 2023

marbemac commented Oct 2, 2023

sapphi-red commented Oct 3, 2023

bluwy left a comment

marbemac commented Oct 3, 2023

sapphi-red left a comment

hyf0 commented Oct 8, 2023 •

edited

Loading

bluwy commented Oct 9, 2023

Tobbe commented Oct 17, 2023 •

edited

Loading

bluwy commented Oct 17, 2023

Tobbe commented Oct 17, 2023

patak-dev commented Oct 18, 2023

feat(ssr): support for ssr.resolve.conditions and ssr.resolve.externalConditions options #14498

feat(ssr): support for ssr.resolve.conditions and ssr.resolve.externalConditions options #14498

Conversation

marbemac commented Sep 28, 2023 • edited Loading

Description

What is the purpose of this pull request?

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

marbemac Sep 29, 2023

Choose a reason for hiding this comment

sapphi-red left a comment

Choose a reason for hiding this comment

sapphi-red Sep 30, 2023

Choose a reason for hiding this comment

marbemac Sep 30, 2023

Choose a reason for hiding this comment

sapphi-red Oct 1, 2023

Choose a reason for hiding this comment

marbemac Oct 2, 2023

Choose a reason for hiding this comment

marbemac commented Oct 2, 2023

sapphi-red commented Oct 3, 2023

bluwy left a comment

Choose a reason for hiding this comment

marbemac commented Oct 3, 2023

sapphi-red left a comment

Choose a reason for hiding this comment

hyf0 commented Oct 8, 2023 • edited Loading

bluwy commented Oct 9, 2023

Tobbe commented Oct 17, 2023 • edited Loading

bluwy commented Oct 17, 2023

Tobbe commented Oct 17, 2023

patak-dev commented Oct 18, 2023

marbemac commented Sep 28, 2023 •

edited

Loading

hyf0 commented Oct 8, 2023 •

edited

Loading

Tobbe commented Oct 17, 2023 •

edited

Loading