Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

docs: optimizeDeps.needsInterop #13323

Merged
merged 3 commits into from
May 24, 2023
Merged

docs: optimizeDeps.needsInterop #13323

merged 3 commits into from
May 24, 2023

Conversation

patak-dev
Copy link
Member

Fixes #13317

Description

Reference #7568


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

@stackblitz
Copy link

stackblitz bot commented May 23, 2023

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@patak-dev patak-dev added the documentation Improvements or additions to documentation label May 23, 2023
- **Experimental**
- **Type:** `string[]`

Forces ESM interop when importing these dependencies. Some legacy packages advertise themselves as ESM but use `require` internally. Adding these packages to `needsInterop` can speed up cold start by avoiding full-page reloads. You'll receive a warning if one of your dependencies is triggering a reload, suggesting to add the package name to this array in your config.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

@bluwy bluwy May 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

config.logger.warn(
`Mixed ESM and CJS detected in ${colors.yellow(
needsInteropMismatch.join(', '),
)}, add ${
needsInteropMismatch.length === 1 ? 'it' : 'them'
} to optimizeDeps.needsInterop to speed up cold start`,
{
timestamp: true,
},
)

const needsReload =
needsInteropMismatch.length > 0 ||

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.

Copy link
Member Author

@patak-dev patak-dev May 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@patak-dev patak-dev merged commit b34e79c into main May 24, 2023
@patak-dev patak-dev deleted the docs/needs-interop branch May 24, 2023 13:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

optimizeDeps.needsInterop is missing from docs
2 participants