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

[SSR] Vite should use the Node.js built-in resolver instead of using a custom resolver #4447

Closed
brillout opened this issue Jul 30, 2021 · 5 comments

Comments

@brillout
Copy link
Contributor

brillout commented Jul 30, 2021

For bundling server-side code, Vite needs to resolve all SSR dependencies.

Today Vite uses a custom resolver.

I'm proposing to use Node.js's built-in require.resolve instead. (With const require = createRequire(import.meta.url) for ESM.)

Today, the vast majority of SSR users are using Node.js servers without a server-side bundler. This means that the de-facto standard resolver today is Node.js's built-in resolver.

Maybe Node.js's resolver would even work for bundling browser-side code, since Node.js now supports ESM. But we would need to check whether the Node.js ESM resolver is the same than webpack's ESM resolver. (It should be in principle since ESM is specced.)

But for the server-side, using Node.js's resolver will de-facto work.

@patak I believe there is a plan to use ESBuild to bundle, is this still true?

@aleclarson

It's not really an issue if Vite handles it properly. Besides, that would prevent resolve.dedupe and mode from affecting module resolution in SSR, which isn't preferable.

I believe we can still support resolve.dedupe (we can still dedupe, regardless of how we resolve) and mode (I'm not sure what is the problem you see here but we can always inject stuff in the bundle, regardless of how we resolve).

@Shinigami92
Copy link
Member

Also looks like https://nodejs.org/api/modules.html#modules_require_resolve_request_options is supported by all our supported node versions

@aleclarson
Copy link
Member

aleclarson commented Jul 30, 2021

There's a reason resolve.dedupe is in the resolve option namespace. It relies on module resolution. :)
Of course, you can always use flat dependencies as a workaround, but that doesn't account for PNPM and yarn link users, since symlinks often lead to duplicates.

Having ssrLoadModule respect the mode option is crucial, as it affects whether the development or production condition is used when resolving the exports field of a package.json module. Unless I'm missing something, this can't be reproduced with process.env.NODE_ENV, but maybe there's another workaround (haven't looked).

@brillout
Copy link
Contributor Author

brillout commented Aug 2, 2021

@aleclarson

That seems like a reason to fix the resolution algorithm

Yes fixing Vite's resolver would be an alternative but I'm concerned that it may take a while until we hunt down all discrepancies between Node.js's resolver and Vite's resolver we are currently experiencing.

I don't know wether the two points you mentioned are fundemantal blockers or there are ways to solve/circumvent them but if we can use Node.js's resolver that'd be neat as it would be a bullet-proof solution.

There's a reason resolve.dedupe is in the resolve option namespace. It relies on module resolution. :)
Of course, you can always use flat dependencies as a workaround, but that doesn't account for PNPM and yarn link users, since symlinks often lead to duplicates.

From a high-level perspective, I'm not sure I see an inherent contradiction between using Node.js's require.resolve and Vite's resolve.dedupe feature. Couldn't we resolve all dependencies with Node.js's require.resolve and then dedupe? A duplicated dependency can be resolved with require.resolve and Vite can still decide to load only one (precisely speaking: Vite woud transform both import statements to the same resolved deduped path).

Having ssrLoadModule respect the mode option is crucial, as it affects whether the development or production condition is used when resolving the exports field of a package.json module. Unless I'm missing something, this can't be reproduced with process.env.NODE_ENV, but maybe there's another workaround (haven't looked).

Ok I see. Although I'm wondering: if it's a feature that Node.js doesn't support then how many server-side libraries are using it?

E.g. Firebase's server-side library has not been designed with Vite in mind so it certainly doesn't use it.

I'm guessing that there aren't that many server-side libraries (I'm guessing only a handful max) using it and it could be worth it to stop supporting the development and production export conditions (for server-side libraries).


It seems that you and Evan are the most familiar with Vite's current resolve algorithm. Maybe there is a blocker I'm not seeing, but it would be great to be able to simply use Node.js's require.resolve as it would get rid of many SSR problemes.

Thoughts?

Looking forward to see Vite evolve into a rock-solid tool :-).

@benmccann
Copy link
Collaborator

benmccann commented Sep 30, 2021

The existing resolve code is currently using options for extensions and preserveSymlinks:

return resolve.sync(id, {

I don't see equivalent options in the built-in Node resolver. Is there a way to support that or do we not actually need those options?

@brillout
Copy link
Contributor Author

brillout commented Oct 1, 2021

There is https://nodejs.org/api/modules.html#modules_require_extensions but it's deprecated.

So I guess we can't use Node.js resolver.

@brillout brillout closed this as completed Oct 1, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Oct 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants