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

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

👍 (Super looking forward to this.)

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

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) 
 } 

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.

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.

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

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.

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

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.