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
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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:So named ESM imports of these packages would never quite work without interop like so:
Some CJS packages are nice enough and exports things nicely like:
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.
There was a problem hiding this comment.
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 likeforcedInterop
it could be more clear. What this option is doing is overriding Vite'sneedsInterop
detection for cases where it fails. See #1724 (comment). This is why I wrote: "Some legacy packages advertise themselves as ESM but userequire
internally.". I reword the description a bit. Does it sound better now?There was a problem hiding this comment.
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?vite/packages/vite/src/node/optimizer/optimizer.ts
Lines 457 to 466 in e3db771
vite/packages/vite/src/node/optimizer/optimizer.ts
Lines 349 to 350 in e3db771
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.
There was a problem hiding this comment.
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 haveneedsInterop
defined, we compute it usinges-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 callinges-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 avoides-module-lexer
for ESM deps.There was a problem hiding this comment.
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:
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:There was a problem hiding this comment.
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 ❤️