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: respect resolve.conditions during nodeResolve #13487

Closed
wants to merge 1 commit into from

Conversation

nksaraf
Copy link

@nksaraf nksaraf commented Jun 11, 2023

Description

Respect user's resolve.conditions while doing the internal node resolve. This is required by "react-server" components. They need the internal node resolve to also use the "react-server" condition. I am passing it in the vite config but its not being used by the nodeResolve function. This adds it to the override conditions, so its used by the nodeResolve function.

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.
  • 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.

@stackblitz
Copy link

stackblitz bot commented Jun 11, 2023

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

@nksaraf nksaraf changed the title respect resolve.conditions during nodeResolve fix: respect resolve.conditions during nodeResolve Jun 11, 2023
patak-dev
patak-dev previously approved these changes Jun 11, 2023
Copy link
Member

@patak-dev patak-dev left a comment

Choose a reason for hiding this comment

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

We can try this during the 4.4 beta. @sapphi-red, I recall you stating this was good for you too. I'll run ecosystem-ci for reference, don't worry about the fails, we have several projects CIs to work out before the 4.4 release.

@patak-dev
Copy link
Member

/ecosystem-ci run

@vite-ecosystem-ci
Copy link

vite-ecosystem-ci bot commented Jun 11, 2023

📝 Ran ecosystem CI: Open

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

@patak-dev
Copy link
Member

Same failures in ecosystem-ci as in main

@sapphi-red
Copy link
Member

I have one thing that I want to discuss before merging this PR. Is it fine to use resolve.conditions here?
I think it's not rare to want to add a condition only for SSR. It's not possible to do that with this PR's implementation. I wonder if adding ssr.resolve.conditions is better here.

IIUC react-server condition should only be used for SSR and not for non-SSR. Though, in the RSC case, RSC requires multiple Vite servers for now and because of that, ssr.resolve.conditions isn't necessary.

https://discord.com/channels/804011606160703521/1097227364846534656/1098923455656759386

@sapphi-red
Copy link
Member

Maybe ssr.resolve.conditions is a confusing name. ssr.resolve.externalConditions might be better. The problem is that this part of the code works like a Loader.

@patak-dev
Copy link
Member

Thanks @sapphi-red. I forgot about the discussion about ssr.resolve.conditions (cc @bluwy too). Would you explain a bit more why externalConditions would be a better name here, though? resolve.conditions are additional allowed conditions (it may have been better to name it resolve.additionalConditions)

I added the PR to the team board so we get to it if don't manage to merge it before Thursday. It would be good to merge this in 4.4, we could add it as experimental if needed.

@patak-dev patak-dev added this to the 4.4 milestone Jun 13, 2023
@patak-dev patak-dev added p3-downstream-blocker Blocking the downstream ecosystem to work properly (priority) feat: ssr labels Jun 13, 2023
@sapphi-red
Copy link
Member

ssrLoadModule works in two steps.

  1. Rewrite the import statements by the plugin pipeline (This is corresponds to the bundling step in the traditional tools): transformRequest call in instantiateModule
  2. Resolve the import statements (This is corresponds to the resolution by the JS runtime): ssrImport call in instantiateModule

For example, if you have import 'foo' and

  • you want to bundle that import, that import should be rewritten by the plugin pipeline
    • otherwise, rollup won't be able to resolve that file
  • you want to externalize that import, that import should not be rewritten by the plugin pipeline, but should be resolved by the ssrImport
    • otherwise, rollup will rewrite the import to the actual path. Also Vite won't be able to load that file.

If the option is named ssr.resolve.conditions, I guess people will think as an option to configure the former one. But what we are going to add is an option to configure the latter one.

@patak-dev
Copy link
Member

Thanks for the write up @sapphi-red! I agree with you then we better call it ssr.resolve.externalConditions. For completeness, given that we have ssr.external, other naming options could be:
a. flatten to ssr.externalConditions
b. ssr.externalResolve.conditions
c. ssr.external: { deps: [...], resolve: { conditions: [...] }

b and c could be interesting if we end up adding other resolve options for ssrImport in the future. I'm fine going with ssr.resolve.externalConditions though

@nksaraf
Copy link
Author

nksaraf commented Jun 13, 2023

But I think we want it to apply to both. Whatever usual resolution steps need to happen (even for bundled stuff) should be resolved with these conditions . I can choose to not externalize a dependency and that should also respect resolve.conditions. I am assuming some vite plug-in is doing that coz I think I saw a PR related to that

@sapphi-red
Copy link
Member

IIRC if the file isn't externalized, resolve.conditions will be used. Though it will be used for both SSR and non-SSR, so I think we need a different option for that too.

I think we need two options here.

  • ssr.resolve.conditions: the one that will be used in the plugin pipeline. the default value is same with resolve.conditions. this option will be used to use different conditions for SSR and non-SSR
  • ssr.resolve.externalConditions: the one that will be used in ssrImport (the one this PR is changing). the default value is [].

Because using ssr.resolve.conditions doesn't require the bundled code to be run with --conditions flag, but using ssr.resolve.externalConditions requires that.


Having ssr.resolve.externalConditions under ssr.external (c.) looks better than having it under ssr.resolve. Since this option requires the runtime (Node.js) to be configured for the bundled code to run.

@bluwy
Copy link
Member

bluwy commented Jun 15, 2023

Yeah I think I agree with sapphi here, we need to create ssr.resolve.conditions and ssr.resolve.externalConditions for this to work nicely. Maybe we can combine both into just the former? In what case would the conditions differ?

In the future, maybe we can use import.meta.resolve instead to simplify the external condition part, but it looks like for Vite 6.

@nksaraf
Copy link
Author

nksaraf commented Jun 15, 2023

Makes sense to me. Better to be explicit for all three cases im understanding:

resolve.conditions: non-SSR
ssr.resolve.conditions: SSR non-externalized (defaults to resolve.conditions)
ssr.resolve.externalConditions: SSR externalized

Any guidance on where the other parts will be implemented. Should I update this PR to handle these?

@sapphi-red
Copy link
Member

In what case would the conditions differ?

For example, module condition can be used for ssr.resolve.conditions, but not for ssr.resolve.externalConditions. module condition is used like a replacement of module field that points to faux ESM files. So if module condition is set to ssr.resolve.externalConditions, the script will throw an error.

In the future, maybe we can use import.meta.resolve instead to simplify the external condition part, but it looks like for Vite 6.

Yeah, maybe. If we use that, users will need to run Vite with node --conditions custom node_modules/vite/bin/vite.js or NODE_OPTIONS='--conditions custom' npm run dev.

Any guidance on where the other parts will be implemented.

ssr.resolve.conditions needs to be used in this function.

function resolveExportsOrImports(
pkg: PackageData['data'],
key: string,
options: InternalResolveOptionsWithOverrideConditions,
targetWeb: boolean,
type: 'imports' | 'exports',
) {
const additionalConditions = new Set(
options.overrideConditions || [
'production',
'development',
'module',
...options.conditions,
],
)
const conditions = [...additionalConditions].filter((condition) => {
switch (condition) {
case 'production':
return options.isProduction
case 'development':
return !options.isProduction
}
return true
})
const fn = type === 'imports' ? imports : exports
const result = fn(pkg, key, {
browser: targetWeb && !additionalConditions.has('node'),
require: options.isRequire && !additionalConditions.has('import'),
conditions,
})
return result ? result[0] : undefined
}

Should I update this PR to handle these?

I think we can split implementing ssr.resolve.conditions to a different PR and focus on ssr.resolve.externalConditions in this PR.

@nksaraf
Copy link
Author

nksaraf commented Jun 19, 2023

ssr.resolve.conditions needs to be used in this function.

Just to be sure, this is actually the function I wanted my passed conditions to go to. Will that be ssr.resolve.externalConditions or ssr.resolve.conditions?

@sapphi-red
Copy link
Member

ssr.resolve.conditions needs to be used in this function.

Just to be sure, this is actually the function I wanted my passed conditions to go to. Will that be ssr.resolve.externalConditions or ssr.resolve.conditions?

Ah. If the function is called from resolvePlugin, ssr.resolve.conditions needs to be used. If the function is called from ssrImport, ssr.resolve.externalConditions needs to be used.

@Tobbe
Copy link

Tobbe commented Jul 26, 2023

Just found this issue after being stuck on trying to get react-server condition working in Redwood for a couple of days. I was 100% sure there was something wrong in my code, until I started following the execution path down into Vite sources in my node_modules 🙂

Looking forward to seeing this fix merged!

Tobbe added a commit to redwoodjs/redwood that referenced this pull request Jul 27, 2023
Use experimental node loaders when launching the RSC FE Server

This is all done to try to make loading of 3rd party modules work.
Specifically have been experimenting with the `server-only` package.

In the end it turned out that it's not working because there's a
bug/missing feature in Vite
vitejs/vite#13487
@Tobbe
Copy link

Tobbe commented Aug 28, 2023

UPDATE:
I've successfully used this code with yarn patch package to do import server-only in a RSC component

ORIGINAL POST:
I tried this code in Redwood using yarn patch package
When I do I get this error in my browser console
TypeError: (0 , u.createFromFetch) is not a function
image

Maybe/probably something else I'm doing wrong, but just wanted to let you all know

Actually, things error out as soon as I add

resolve: {
  conditions: ['react-server'],
}

even without this patch.
But I do get a different (very similar) error
image

Tobbe added a commit to redwoodjs/redwood that referenced this pull request Sep 16, 2023
By configuring `noExternal` for the server side build correctly we now
support using npm modules with client side only React components, i.e.
components with `'use client'` at the top of the file.

Currently you have to patch vite for this to work. Still waiting for
vitejs/vite#13487 to be merged.
Tobbe added a commit to redwoodjs/redwood that referenced this pull request Sep 17, 2023
This is needed until vitejs/vite#13487 is merged
and released and RW uses that new version of Vite
himself65 added a commit to himself65/vite that referenced this pull request Sep 20, 2023
himself65 added a commit to himself65/vite that referenced this pull request Sep 20, 2023
@marbemac
Copy link
Contributor

marbemac commented Oct 5, 2023

@nksaraf I went ahead and implemented the new ssr.resolve options in #14498 - hopefully they help!

@bluwy
Copy link
Member

bluwy commented Oct 6, 2023

I'll close this in favour of #14498

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
Archived in project
Development

Successfully merging this pull request may close these issues.

6 participants