refactor: simplify resolveSSRExternal

[benmccann]

This is a rebased version of #5210 from @aleclarson, which looks good to me

Description

1. add collectExternals function for recursive tracing
2. apply config.ssr just once, after all externals are collected
3. support packages with deep imports but no main entry

To clarify #​3, a package might allow imports like foo/bar but not foo alone. Before this PR, packages like this were never marked external (unless done so explicitly in user config), because the call to tryNodeResolve would throw. As a workaround, you can add the package's name to ssr.external or add an empty index.js module to the package.

[aleclarson]

Thanks for rebasing. I will let @patak-js merge when he's ready

[patak-dev]

I'll bring this PR to our next team meeting for discussion 👍🏼

[yyx990803]

Let's get this out in a beta and test in relevant SSR-reliant projects.

/cc @ElMassimo @frandiox @brillout @FredKSchott

[brillout]

👍 (Super looking forward to this.)

@benmccann @aleclarson I'm confused about this block:

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

Lines 126 to 136 in 871f146

	else if (/\.m?js$/.test(entry)) {
	if (pkg.type === "module" || entry.endsWith('.mjs')) {
	ssrExternals.add(id)
	continue
	}
	// check if the entry is cjs
	const content = fs.readFileSync(entry, 'utf-8')
	if (/\bmodule\.exports\b|\bexports[.\[]|\brequire\s*\(/.test(content)) {
	ssrExternals.add(id)
	}
	}

It basically checks: if dep is ESM => externalize and if dep is CJS => externalize. Doesn't that mean that the dep will always be externalized? I.e. why can't we simply do the following instead?

 else if (/\.m?js$/.test(entry)) { 
   ssrExternals.add(id) 
 } 

[brillout]

Also, I'm wondering: why does Vite preemptively resolves all SSR externals at once by reading a bunch of package.json#dependencies, instead of evaluating whether an import should be externalized as Vite stumbles upon new import statements? I'm probably missing something here but AFAICT the latter would be much simpler.

[brillout]

Maybe we can add some more comments? I find it difficult to comprehend resolveSSRExternal() and its intention. (Btw. I find Vite's source code actually supringsly and refreshingly easy to read 😊, especially considering the inherent complexities of bundlers.)

 -    entry = tryNodeResolve(
+     esmEntry = tryNodeResolve(
        id,
        undefined,
        resolveOptions,
+       // We set `targetWeb` to `true` to get the ESM entry
        true,
        undefined,
        true
      )?.id

// assume external if not yet seen

Why?

ssrExternals.delete('vite')

Why? I would have assumed that Vite is actually a good candidate to externalize.

[benmccann]

Doesn't that mean that the dep will always be externalized? I.e. why can't we simply do the following instead?

I guess the only other case would be if there's some other JS format like AMD or SystemJS. I don't know how common those are though. I'd be fine assuming we should always externalize JS files, but am not sure I'm the right person to make that call

ssrExternals.delete('vite')
Why? I would have assumed that Vite is actually a good candidate to externalize.

I added comments the other places you suggested. I'm not sure about this one though, so will leave that one to @aleclarson to explain

[aleclarson]

Doesn't that mean that the dep will always be externalized? I.e. why can't we simply do the following instead?

I guess the only other case would be if there's some other JS format like AMD or SystemJS.

In that case, we should add a code comment. I wonder if, instead of checking for CommonJS, we should check for signs of AMD or SystemJS..?

 

ssrExternals.delete('vite')

Why? I would have assumed that Vite is actually a good candidate to externalize.

See 578c591 and #1865

 

Also, I'm wondering: why does Vite preemptively resolves all SSR externals at once by reading a bunch of package.json#dependencies, instead of evaluating whether an import should be externalized as Vite stumbles upon new import statements?

While making this PR, I tried removing the knownImports argument of resolveSSRExternal, but I ran into some bugs, so I added it back without really understanding why transitive imports needed to be included.

In regard to package.json crawling, I didn't really question it. Now that I am... it seems like we could refactor it to take more of a lazy approach, since only the vite:import-analysis plugin currently uses server._ssrExternals, and #5220 will also use it once merged. For now though, the current approach gets the job done.

[benmccann]

I added a comment about why vite is removed from externals. I feel like the other stuff is bigger and would be better to do in separate PRs

[brillout]

I'd be fine assuming we should always externalize JS files

Since we'll release this PR as a beta, I'd say that it is a good opportunity to try this out.

It would get us closer to the vision of:

1. Always externalize SSR deps by default.
2. Only transpile user code to CJS if the user's package.json#type is not module.

LGTM otherwise, thanks for the comments super helpful.

[patak-dev]

Thanks for pushing this forward. This PR is already quite involved, let's get this in and test it during the next few days (we'll release 2.7.0-beta.2 tomorrow including it). We can then continue to build on top of it on other PRs so it is easier to discuss and track each proposal.