fix: always externalize ESM and CJS

[benmccann]

Description

fixes #5890

Additional context

@brillout had suggested this change in #5544 (comment). I wanted to punt on it at the time because it was an extra change I wasn't sure about. However, we've now run into a bug report that would fix and I've had more time to think about the suggestion and become more confident in it.

Some of the lines being removed were introduced in 2.7 via #5197

---

What is the purpose of this pull request?

• Bug fix
• New Feature
• Documentation update
• Other

Before submitting the PR, please make sure you do the following

• Read the Contributing Guidelines.
• Read the Pull Request Guidelines and follow the Commit Convention.
• Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
• Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
• Ideally, include relevant tests that fail without this PR but pass with it.

[brillout]

👍 I love this. (As you probably already have guessed :-).)

The packages/playground/optimize-missing-deps/ test is failing because the test is erroneous: the first dependency is not a proper ESM-only dependency.

// packages/playground/optimize-missing-deps/package.json 

  "dependencies": {
    "missing-dep": "file:./missing-dep",
    "multi-entry-dep": "file:./multi-entry-dep"
  }

To fix the test we can do one of the following:

• Add type: "module" to missing-dep/package.json
• Rename missing-dep/index.js to missing-dep/index.mjs
• Or rewrite missing-dep/index.js to CJS

[haoqunjiang]

Though the missing-dep case is not a valid ESM dependency (for node), I think it is a case that may be worth supporting.
So I think the safe fix here is as pointed out in #5890 (comment), that pkg.type should refer to the package.json of the dependency.

[haoqunjiang]

At this moment, not many people have figured out how to publish ESM packages correctly for the Node.js native ESM loader to consume.
People may have heard of the exports field and added it to their packages, and the package still works with webpack or tsc, or with plain require, but not with Node.js native ESM loader.
And it's still a small percentage of Node.js developers that are using the native ESM features to import third-party packages, so few libraries have caught up yet.

For example, last I checked, @firebase/app still has an incorrect ESM entry: https://unpkg.com/browse/@firebase/[email protected]/package.json

[brillout]

Though the missing-dep case is not a valid ESM dependency (for node), I think it is a case that may be worth supporting.

Why so? I don't see a reason for supporting code that doesn't and will never exist.

If anything he won't do the ecosystem a favor of supporting code that doesn't work with Node.js: code will emerge that only works with Vite which is bad for interoperability. The whole idea of ESM is to have a single standard. Vite will not do anyone a favor of deviating from that standard.

[brillout]

@sodatea

Yes if the target is the browser. But we are speaking of SSR here.

Today, the only target for SSR is Node.js. We shouldn't create a second target being "SSR code that only works with Vite".

[haoqunjiang]

Anyway, I don't think this fix should be a blocker for 2.7.

• react-markdown was broken before 2.7, so it's not a regression;
• The issue can be fixed in another way
• Dropping support for incorrect ESM entries is a big deal IMO. Even if I'm in favor of the change, let's discuss it in 2.8 or 3.0 and not delay 2.7 further.

[brillout]

Ok, but note that if we still have other regressions then this PR may actually help.

I've been pushing for quite some time for Vite to externalize SSR dependencies. This is an important step in that direction.

[patak-dev]

Ok, but note that if we still have other regressions then this PR may actually help.

If you test the current regressions and this is fixing them, let's review it. If not, I agree with Sodatea here. Let's try to push 2.7 out and then we can include more fixes when testing 2.8 beta, that we may start right away after the release

[brillout]

Where can I find the list of regressions? (Couldn't find a proper (combo of) GitHub issue label.)

[brillout]

(Can't promise anything; I'm super swamped with vite-plugin-ssr and Telefunc and Telefunc's beta release is already way too delayed already.)

[patak-dev]

@brillout they are in this milestone https://github.com/vitejs/vite/milestone/4

Direct links to the two issues there:

#5812

#5706 (you already commented in this one, looks like a PR is actionable)

[benmccann]

For example, last I checked, @firebase/app still has an incorrect ESM entry: https://unpkg.com/browse/@firebase/[email protected]/package.json

@sodatea can you clarify what is wrong with it? We've had lots of complaints about Firebase + SvelteKit and I'd like to send them a PR with a fix if there's something wrong. However, it looks fine to me. It looks like they are using the CJS version for require and the ESM version otherwise, which I think is correct?

"exports": {
  ".": {
    "require": "./dist/index.cjs.js",
    "esm5": "./dist/esm/index.esm5.js",
    "default": "./dist/esm/index.esm2017.js"
  },
  "./package.json": "./package.json"
},

[haoqunjiang]

The package.json doesn't have a type: "module" field, meaning that only files with the .mjs extension can be treated as ES modules, but the ESM entry uses a plain .js extension, therefore, Node.js will fail to parse it.

And that's the case that the original commit intended to avoid (though it used the wrong pkg and caused the bug here)

[benmccann]

Ah, thanks for clarifying. I think just fixing the pkg is the better solution then. If we want to help the ecosystem improve we can log a warning that we detected a malformed package, which seems nicer than crashing the whole server.

[brillout]

@sodatea @benmccann

I strongly believe we should merge this PR.

Both #5927 and this PR fix @firebase/app but this PR is much simpler.

I don't see any reason why we should go for the more complicated solution.

As for showing a warning I don't see that to justify complexifying Vite's source code.