[SSR] Stop resolving externalized dependencies

brillout commented 3 years ago

Describe the bug

AFAICT, Vite should completely ignore externalize SSR dependencies, including:

Not trying to resolve the dependency's module locations
~Not read the dependency's code at all~ (edit: this seems to be the case ✅ )

E.g. https://github.com/vuetifyjs/vuetify/issues/13936; I don't see a reason why Vite should try to resolve the module vuetify/lib/components.

Proposal: Vite should completely ignore externalized deps.

cc @GrygrFlzr

@aleclarson Do you know why Vite touches externalized deps?

@patak-js I'm thinking maybe we should cue in Evan about this at some point.

Reproduction

Vuetify repro: https://github.com/brillout/vuetify3-ssr

brillout commented 3 years ago

After some digging the root cause of the react-pdf-highlighter error is on react-pdf-highlighter's side.

At least Vite doesn't do dep transpilation here, but it still does dep resolving. (I.e. Vite uses it's owner node resolver instead of using Node.js' built-in require/require.resolve.)

Vite doing dep resolving is the root cause of the Vuetify error.

aleclarson commented 3 years ago

I'm not getting the same error when running your reproduction at https://github.com/vuetifyjs/vuetify/issues/13936:

Circular dependency: /Users/alec/dev/sandbox/vuetify3-ssr/node_modules/vite-plugin-ssr/dist/esm/page-files/pageFiles.node.js -> /pages/_default/_default.page.server.js -> /pages/_default/app.js -> /pages/_default/vuetify.js -> /node_modules/vuetify/lib/components/index.mjs -> /node_modules/vuetify/lib/components/VCode/index.mjs -> /node_modules/vuetify/lib/util/index.mjs -> /node_modules/vuetify/lib/util/createSimpleFunctional.mjs -> /node_modules/vuetify/lib/util/index.mjs
Circular dependency: /Users/alec/dev/sandbox/vuetify3-ssr/node_modules/vite-plugin-ssr/dist/esm/page-files/pageFiles.node.js -> /pages/_default/_default.page.server.js -> /pages/_default/app.js -> /pages/_default/vuetify.js -> /node_modules/vuetify/lib/components/index.mjs -> /node_modules/vuetify/lib/components/VTimeline/index.mjs -> /node_modules/vuetify/lib/components/VTimeline/VTimelineItem.mjs -> /node_modules/vuetify/lib/components/VTimeline/VTimelineDivider.mjs -> /node_modules/vuetify/lib/components/index.mjs
3:56:56 PM [vite] Error when evaluating SSR module /node_modules/vuetify/lib/components/VBanner/VBannerActions.mjs:
TypeError: Cannot read property 'defineComponent' of null
    at Module.createSimpleFunctional (../../src/util/createSimpleFunctional.ts:9:24)
    at ../../../src/components/VBanner/VBannerActions.ts:3:37
    at instantiateModule (/Users/alec/dev/sandbox/vuetify3-ssr/node_modules/vite/dist/node/chunks/dep-11db14da.js:73323:166)

So it seems like vuetify/lib/components is resolved just fine.

Do you know why Vite touches externalized deps?

I don't think it does? But not clear on your definition of "touches" here.

brillout commented 3 years ago

@aleclarson Yes that's because of this commit https://github.com/brillout/vuetify3-ssr/commit/9ed87bb693ed1b083bb59755aede83da165bc4f5 as it helps Vite's custom resolver.

I don't think it does? But not clear on your definition of "touches" here.

I wasn't sure but actually Vite doesn't seem to transpiled externalized deps (correctly as expected).

But it does, however, try to resolve which sometimes fails, e.g. Vuetify. I reverted the commit at https://github.com/brillout/vuetify3 to reproduce:

10:29:09 PM [vite] Error when evaluating SSR module /pages/_default/vuetify.js:
Error: Cannot find module 'vuetify/lib/components' from '/home/romuuu/tmp/vite-plugin-ssr-vuetify/pages/_default'
    at Function.resolveSync [as sync] (/home/romuuu/tmp/vite-plugin-ssr-vuetify/node_modules/resolve/lib/sync.js:102:15)
    at resolveFrom$3 (/home/romuuu/tmp/vite-plugin-ssr-vuetify/node_modules/vite/dist/node/chunks/dep-11db14da.js:4076:29)
    at resolve (/home/romuuu/tmp/vite-plugin-ssr-vuetify/node_modules/vite/dist/node/chunks/dep-11db14da.js:73358:22)
    at nodeRequire (/home/romuuu/tmp/vite-plugin-ssr-vuetify/node_modules/vite/dist/node/chunks/dep-11db14da.js:73337:25)
    at ssrImport (/home/romuuu/tmp/vite-plugin-ssr-vuetify/node_modules/vite/dist/node/chunks/dep-11db14da.js:73290:20)
    at eval (/pages/_default/vuetify.js:9:31)
    at instantiateModule (/home/romuuu/tmp/vite-plugin-ssr-vuetify/node_modules/vite/dist/node/chunks/dep-11db14da.js:73323:166) (x2)

aleclarson commented 3 years ago

But it does, however, try to resolve which sometimes fails

That seems like a reason to fix the resolution algorithm, rather than treat externalized deps differently when resolving them.

Have you seen if vitejs/vite#3951 fixes it?

brillout commented 3 years ago

My points here are:

Why don't we use Node.js' built-in require.resolve() instead of using our own? In a Node.js context, I don't see a reason why we would want to use something else.
Why does Vite try to resolve an externalized dependency in the first place?

aleclarson commented 3 years ago

Why don't we use Node.js' built-in require.resolve() instead of using our own? In a Node.js context, I don't see a reason why we would want to use something else.

Once vitejs/vite#3951 is merged, modules loaded with ssrLoadModule will use their own require. See here.

const loadModule = Module.createRequire(importer || resolveOptions.root + '/')

Why does Vite try to resolve an externalized dependency in the first place?

No reason currently, I think. Vite could just use require for externals. But when vitejs/vite#3951 is merged, custom resolution allows for resolve.dedupe and mode configuration to be respected by all dependencies (compiled or not).

brillout commented 3 years ago

No reason currently, I think. Vite could just use require for externals.

I'd argue that Vite should not try to resolve externalized dependencies then. This is what I expect as a user, and also what the ecocystem at large expects.

Once vitejs/vite#3951 is merged, modules loaded with ssrLoadModule will use their own require. See here.

:+1: Neat. Although I'm still wondering; why doesn't Vite use Node.js's built-in require.resolve instead of implementing a customer resolver? (For SSR.)

benmccann commented 3 years ago

One thing I'll mention that is somewhat SvelteKit-specific is that we need to bundle all third-party Svelte components because they are distributed as source and need to be run through the Svelte compiler. By far the biggest issue we have is that Svelte components typically get distributed as ESM, but if you encounter one with a CJS dependency it will fail. If we can stop doing new Function and include node_modules in a more standard way with require / import this would help tremendously

aleclarson commented 3 years ago

@benmccann Your comment seems out of place here, since this thread is about module resolution, not module loading. Nonetheless, have you tried using esm to compile ESM with CJS dependencies on-demand? I might be wrong, but I don't think Node supports ESM with CJS dependency anyway.

I'd argue that Vite should not try to resolve externalized dependencies then. This is what I expect as a user, and also what the ecocystem at large expects.

@brillout 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.

benmccann commented 3 years ago

Nonetheless, have you tried using esm to compile ESM with CJS dependencies on-demand?

Like patching Vite to do that? What would get compiled? The ESM to CJS? I think getting that heavy into Vite's internals is far beyond my knowledge of Vite. But why would that be required? I think that if we use Node for the resolving logic we should consider using Node for the loading logic as well because Node has built-in logic to handle it already, so why recreate it?

Your comment seems out of place here, since this thread is about module resolution, not module loading

I'm not incredibly familiar with Vite's internals, but in my mind, the two are highly tied together because the reason you need to do module resolution is for module loading. E.g. Vite can't load ESM modules today because it always tries to require them. However, if it detected that a module was ESM you could import it instead. (I wonder if there might be a way around this by distributing Vite as type: "module" so that you can always do import)

I might be wrong, but I don't think Node supports ESM with CJS dependency anyway.

I'm relatively sure that it does: https://nodejs.org/api/esm.html#esm_interoperability_with_commonjs. It's the vitejs/vite#1 thing that users report as far as SSR issues go, so I have to imagine Node supports it based on how many reports of it I've seen. E.g. I just saw that the other day with the unified library: https://github.com/vitejs/vite-plugin-react/issues/30

brillout commented 3 years ago

ESM Plan

The fact that Node.js forces a user to migrate her code from CJS to ESM when she adds even only a single ESM-only dependency is quite brutal. I don't think it's a Node.js design flaw, quite the contrary actually. For example, it's been a month that unified is an ESM-only package and since unified is such a widely used pacakge, it will become a tremendous force for ESM adoption.

I was actualy skeptical that ESM will ever get to mass adoption for Node.js user-code, but I'm now confident ESM will spread everywhere even for Node.js user-code. We only need a couple of widely used packages to become ESM-only, and ESM will soon become the de-facto standard.

Let's imagine I've got a Node.js app written with CJS using unified@9, and I'm not using Vite. I now want to upgrade to unified@10. Because unified@10 is a ESM-only pacakge, I'm forced to:

Add type: module in my package.json.
Migrate my whole code from CJS to ESM.

That's a good thing and that's the whole plan here: ESM is spreading.

Vite should respect that clever plan.

So, the only thing Vite should do here is to check whether the user-code's package.json has type: module and if it's the case then Vite doesn't transpile the user-code's import statements to require. That's it.

If a Vite user wants to use unified@10 she has to add type: module to her package.json. And that's a good thing as it respects the ESM-spreading plan. The good news for Vite users is that step 2 can be skipped since Vite user-code is usually already written in ESM.

Bottom line: Vite doesn't need to resolve externalized SSR dependencies, even for ESM.

Not resolving externalized SSR dependencies will work

But, most importantly: why am I confident that Vite shouldn't resolve externalized SSR dependencies?

Today, the vast majority of SSR users are using Node.js servers without a server-side bundler. This means that Node.js is the de-facto standard server-side environment. That's why if Vite stays out of the way and simply lets Node.js do its job, then things will just work.

We can safely make Vite only touch user-code and not dig into node_modules/. (For dev and for externalized SSR dependencies.)

benmccann commented 3 years ago

The fact that Node.js forces a user to migrate her code from CJS to ESM when she adds even only a single ESM-only dependency is quite brutal

It doesn't quite require it. CJS can do dynamic imports of ESM. It just can't do static imports of ESM

Today, the vast majority of SSR users are using Node.js servers without a server-side bundler.

Just FYI, SvelteKit has adapters for different environments. The Node adapter runs its code through ESBuild after doing the Vite build. People have also build adapters for other platforms like Cloudflare Workers and Deno. Does Vue by default do only client-side rendering? I had the sense the SvelteKit might be one of the primary ways people use SSR, but I don't know whether that's true. I don't quite understand how whether a server-side bundler is being used would affect this decision though, so none of that contradicts the idea that we might not need to resolve external dependencies.

benmccann commented 3 years ago

If a Vite user wants to use unified@10 she has to add type: module to her package.json

SvelteKit projects all have type: "module" by default, but users still can't use unified. If I force unified to be external I get the message Must use import to load ES Module because Vite changed my import statement to require. I know that's a loading issue and this thread is about resolving, but I guess the point I'm trying to make is that it's possible that if we change resolving without fixing loading first it's possible that we're going to be hitting these loading issues way more often making SSR even more difficult to use. Not trying to be a cold blanket here because I support your plan. Just trying to keep the larger picture in mind and order things appropriately

brillout commented 3 years ago

@benmccann

velteKit projects all have type: "module" by default, but users still can't use unified. If I force unified to be external I get the message Must use import to load ES Module because Vite changed my import statement to require.

If the user has type: "module", then Vite should certainly not transpile import statements to require. That's part of the ESM plan I described above.

This means that SveleKit users will be able to use unified (since the user's code does import statements even after transpilation).

The neat thing here is: Vite doesn't need to resolve externalized SSR dependencies. Vite just looks at the user's package.json and if there is a type: "module" then it keeps import statements and otherwise transforms them into require statements. That's it.

To sum up:

Main propsal: Vite doesn't need to resolve externalized SSR dependencies.
ESM proposal: Vite shouldn't transform import statements to require statements if the user has type: "module".
Remove custom resolver propsal #4447: for bundling the entire server-side code with all dependencies, Vite needs to resolve all dependencies. But maybe we can use Node.js's resolver and remove Vite's custom resolver (for SSR at least; we may still need it for the browser-side).

I believe these three proposals will solve many (if not most...) SSR problems mentioned in https://github.com/vitejs/vite/discussions/4230.

Let me know if there are doubts/questions, I'm happy to elaborate further.

vitejs / vite