When I last investigated needsInterop, I realized this isn't the case why some packages needs interop. I found it's because when bundling CJS packages, it's not always possible to get all the possible export names. You could have a CJS package that's:

module.exports = require('./somewhere')
// or
module.exports[Math.random()] = 'why not'

So named ESM imports of these packages would never quite work without interop like so:

import * as _pkg from 'pkg'
const { something } = _pkg

Some CJS packages are nice enough and exports things nicely like:

module.exports.foo = 'bar'
module.exports.nice = 'bundle'

And I believe those work fine because esbuild is able to analyze them. So maybe we should take the opportunity to update this so we don't misleadingly point that other packages are not exporting stuff right.

This is the reason why needsInterop is needed in general, and Vite automatically detects when a dependency needs interop. Even in your last case, IIUC, esbuild will end up wrapping it as a CJS module and we'll apply interop (for example for React).

Maybe if we would call optimizeDeps.needsInterop something like forcedInterop it could be more clear. What this option is doing is overriding Vite's needsInterop detection for cases where it fails. See #1724 (comment). This is why I wrote: "Some legacy packages advertise themselves as ESM but use require internally.". I reword the description a bit. Does it sound better now?

Hmm I thought we were already able to detect these kind of packages by default (#11502), and needsInterop is only an optimization for faster cold starts?

So with that I think the needsInterop term makes sense, because in most cases users are adding "Vite auto-detected but suggested as an explicit config" dep names to it.

If we do still want to keep the "legacy packages" part, I don't mind too much (maybe it's still true but I haven't seen it being used manually) and the updated one looks good to me.

I think the need for the config option is still independent of esbuild being able to avoid the CJS wrapper for moment. As I understand the code right now, we first try to guess if interop is needed. We call this function during import analysis, and if we don't yet have needsInterop defined, we compute it using es-module-lexer.

Then, once esbuild has processed all the entry points. We have exports information, so we're able to know if is a CJS wrapper or not to decide if interop is needed.

If our initial guess was right, then all good. If there is a mismatch, we do a full-page reload. So optimizeDeps.needsInterop would currently only help if there is a mismatch. We could avoid calling es-module-lexer though if the dep is in the array, but it will be an optimization only for the CJS case, it would be a lot better to know that something doesn't need interop instead so we could avoid es-module-lexer for ESM deps.

Thanks for the explanation! In that case the mismatch would happen because of this part here IIUC. And that part is a bit hard to explain 🤔 But I think it's more accurate than the current:

some legacy packages advertise themselves as ESM but use require internally

I also checked the original issue with react-virtualized and it seems to be packaged right without the mixed require in ESM. What about something like:

Forces ESM interop when importing these dependencies. Vite is able to properly detect when a dependency needs interop, so this option isn't generally needed. However, different combinations of dependencies could cause some of them to be prebundled differently. Adding these packages to needsInterop can speed up cold start by avoiding full-page reloads. You'll receive a warning if this is the case for one of your dependencies, suggesting to add the package name to this array in your config.

Sounds good! Thanks for checking these updates so carefully ❤️