fix(ssr): use hookNodeResolve only when resolve.dedupe exists

[aleclarson]

Description

Members of the ecosystem (eg: @bluwy, @natemoo-re, @dominikg, @brillout) have expressed regret about Vite hooking into the CJS module resolution (via hookNodeResolve) and using its own tryNodeResolve function for bare imports. This PR tries to minimize the impact of that approach.

We still want to respect resolve.dedupe but also defer to Node's built-in resolver whenever possible. To achieve this, I've inlined the deduping logic and it's only applied when the importer exists outside the project root (ie: a package linked to a local clone). This assumes the project root will never contain multiple copies of the same dependency, which I believe is a fine assumption since Semver ranges and package managers typically ensure that. And there's always pnpm.overrides if all else fails.

The mode option

Another motivation for hookNodeResolve is respecting the mode option. For example, if mode: "development" is used, the rationale is that SSR should prefer development entries in pkg.exports field as well. After discussing this point with @bluwy, I've decided it makes more sense for Vite SSR to use the same conditions as the Node.js runtime is using (except for SSR builds), which will ensure identical resolution, regardless of whether the importer is externalized or not.

This is a proof of concept

Testing is yet to be done. Idk if Vite's test suite covers all our bases or not, and I don't have time to contribute new tests unfortunately. If anyone in our ecosystem can manually test the edge cases that are important to them, it would be much appreciated!

---

What is the purpose of this pull request?

• Bug fix
• New Feature
• Documentation update
• Other

[patak-dev]

LGTM. The failing test looks like unused variables only.
We should merge and test with the vite-ecosystem-ci, Astro, vite-plugin-ssr and Hydrogen at least, and we should include it as part of a minor to be safe. Maybe we are still on time for adding it in 2.8 (if we merge next week and wait a few days or a week for the release). If not, let's target 2.9 for this change. As @bluwy expressed, there is no rush at this point.

[aleclarson]

Unsure if this check if compatible with Windows..

[dominikg]

good question. I'd love to have 2 things (though that's better discussed on discord)

1. hopefully get rid of windows path issues once and for all by using pathe by @pi0
2. a complete util for conversion of id to path to url, usable within vite and plugins alike (that also covers working with url params, eg by exposing url as URL, or maybe typing params as a map with a few blessed keys (t, raw etc).

[aleclarson]

we should discuss this with Patak when he's back from vacation 👍

[aleclarson]

Any idea if this is still needed, @natemoo-re ?

[natemoo-re]

I would assume this is still needed for Windows compat... but if the tests covering this are passing, I guess not!

[dominikg]

vite-plugin-svelte injects svelte itself into dedupe, so dedupe will always exist in sveltes case.

If deduplicating across linked cjs packages is the only remaining usecase for tryNodeResolve I wonder if that's worth all this risk and effort.

Is there a different way to solve this problem?
What are the conditions where someone has a setup like this and could we simply ask them to take configurative measures to deduplicate manually via extra linking?

edit: I should probably state that me asking all these questions is trying to find the best way forward and improve my understanding. I'm very happy and thankful to see you working so hard on this.

[aleclarson]

could we simply ask them to take configurative measures to deduplicate manually via extra linking?

Maybe, but in the case where, say, a React library you've forked is being used by two of your Vite projects, you'd have to either maintain multiple clones (one per Vite project), update the symlink when switching to the other Vite project, or maintain a clone of React itself that all of your library clones (and Vite projects) are linked to. It's all very burdensome when I believe the current solution should work perfectly for everyone.

I wonder if that's worth all this risk and effort.

It's not much risk or effort anymore, since we no longer use tryNodeResolve. 🤷

[brillout]

Not familiar with this but would something like the following work?

hookResolve(id => {
  if (id === 'react') {
    require.resolve('react', {
      // `root` is the user's project root directory
      paths: [ root ]
    })
  }
})

This means that React is always resolved to the user's React installation. I.e. we dedupe React and still use Node.js' built-in resolver.

Maybe we could even make @vitejs/plugin-react to enable this mechanism by default?

[dominikg]

oooh, i like that idea. I remember experimenting with aliases to force deduplicate. So what if plugin react returned an alias for react from its config hook?

But from a more general perspective this isn't just a problem for plugin react. So maybe a utility plugin vite-plugin-forced-resolve could do it instead?

[brillout]

I only know React that crashes when there is more than one React instance loaded.

Do we need dedupe for any library other than React?

As for React, I'm wondering: why would the user want something else than react to always resolve to require.resolve('react', { paths: [ root ] })? This seems like not only a sensible default, but also a good thing to enforce?

Maybe I'm missing something here, but I think it's worth it to try to push back on adding functionalities we may not need.

[bluwy]

Is it fine that we're running this for SSR dev only? Otherwise when we run an SSR build and bundle a node package, it would skip this part, which I think causes inconsistencies in dev mode.

[aleclarson]

We can't know at build time which --conditions flags will be used when running the SSR build. :\

[bluwy]

Oh true that's tricky 🤔 Perhaps we should only apply this code for externalized packages in dev mode only? Since in SSR build, non-externalized packages will use Vite's resolve options. That way we can reduce the inconsistencies. Seems like you've commented against this behaviour in-code though, what do you think?

[aleclarson]

To ensure resolution is consistent (externalized or not) in SSR builds, we would need to add a ssr.conditions option and add a runtime warning when the --conditions flags don't match it. Or there might be a way to inject --conditions flags at runtime automatically? Idk.

[aleclarson]

Maybe the best option is (as @brillout has suggested before) to avoid Vite's resolution algo for imports of externalized packages from non-externalized modules as well. If we do that, then it won't matter if there's inconsistencies between Node's runtime resolution and Vite's build-time resolution, since they (practically speaking) never resolve to the same package (and so mismatched conditions have no confusing effects).

[bluwy]

Maybe the best option is (as @brillout has suggested before) to avoid Vite's resolution algo for imports of externalized packages from non-externalized modules as well

Yeah this sounds like a fine plan too. I don't think non-externalized modules should rely on conditions though since that usually means the user explicitly wants to bundle them using Vite's resolve terms. For externalized modules, we can leave it as is so the environment platform resolves it themselves.

[aleclarson]

Do we need dedupe for any library other than React?

Any library with global state. And anything listed in resolve.dedupe ;P

Not familiar with this but would something like the following work?

hookResolve(id => {
  if (id === 'react') {
    require.resolve('react', {
      // `root` is the user's project root directory
      paths: [ root ]
    })
  }
})

This means that React is always resolved to the user's React installation. I.e. we dedupe React and still use Node.js' built-in resolver.

We could use the paths option instead of Module.createRequire, though I don't see the practical difference?

This PR is already doing as you say; resolving deduped modules relative to the project root, then passing that resolved path on to Node's module resolution. Anything not deduped is left alone. 😏

[brillout]

Any library with global state. And anything listed in resolve.dedupe ;P

Many libraries with a global state do not need resolve.dedupe; a library can have a single global state while having duplicated instances by using globalThis.

So, I'm still wondering: is there any library other than React that needs deduplication?

We could use the paths option instead of Module.createRequire, though I don't see the practical difference?

I wasn't aware of Module.createRequire, so I guess there isn't much. Although, require.resolve is more readable and probably better known.

we no longer use tryNodeResolve.

I believe we can also stop using it in ssrExternal.ts. By following my beloved :-) proposal of externalizing SSR deps by default.

I'm not sure if we can also get rid of it at plugins/resolve.ts? If we do then we can completely remove tryNodeResolve. That'd make things dramatically more robust.