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.

Sign up for GitHub

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

Jump to bottom

fix: interop of cjs with_esModule is consistent with rollup #16437

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

fix: interop of cjs with_esModule is consistent with rollup #16437

wants to merge 4 commits into from

Conversation

XiSenao
Copy link
Collaborator

@XiSenao XiSenao commented Apr 16, 2024 •
edited

Description

CJS packages with __esModule require consistent interop behavior with rollup during development phase.

@stackblitz StackBlitz
Copy link

stackblitz bot commented Apr 16, 2024

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

@XiSenao XiSenao added p3-minor-bug An edge case that only affects very specific usage (priority) inconsistency Inconsistency between dev & build p2-edge-case Bug, but has workaround or limited in scope (priority) and removed p3-minor-bug An edge case that only affects very specific usage (priority) labels Apr 17, 2024
@sapphi-red sapphi-red added has workaround feat: deps optimizer Esbuild Dependencies Optimization labels Apr 17, 2024
sapphi-red
sapphi-red previously approved these changes Apr 17, 2024
View reviewed changes
Copy link
Member

@sapphi-red sapphi-red left a comment •
edited

Choose a reason for hiding this comment

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

The change looks good to me. Would you add a test in playground/optimize-deps?

@sapphi-red

This comment was marked as outdated.

Sign in to view
@vite-ecosystem-ci

This comment was marked as outdated.

Sign in to view
@XiSenao XiSenao force-pushed the fix/default-interop-consistency branch from 02d6402 to aeaf129 Compare April 18, 2024 06:23
@XiSenao
Copy link
Collaborator Author

XiSenao commented Apr 18, 2024

/ecosystem-ci run

@XiSenao XiSenao changed the title fix: default imported interop is synchronized with rollup. fix: interop of cjs with_esModule is consistent with rollup Apr 18, 2024
@vite-ecosystem-ci
Copy link

📝 Ran ecosystem CI on aeaf129: Open

suite result latest scheduled
nuxt ✅ success ❌ failure

✅ analogjs, astro, histoire, ladle, laravel, marko, previewjs, quasar, qwik, rakkas, remix, sveltekit, unocss, vike, vite-plugin-pwa, vite-plugin-react, vite-plugin-react-pages, vite-plugin-react-swc, vite-plugin-svelte, vite-plugin-vue, vite-setup-catalogue, vitepress, vitest

@bluwy
Copy link
Member

bluwy commented Apr 18, 2024 •
edited

I wonder if the Rollup behaviour is correct though. __esModule means that if you default import a module, you'd access {}.default. If we have to check whether .default exist, then why would the module mark __esModule in the first place, and I think the reason is that it doesn't want to allow modules to default import it instead, only named imports. And so the current behaviour is correct, and we should improve the error message instead?

For consistency with Rollup, I'm ok if we want to go ahead with this first, but I think we should re-evaluate if this is the correct behaviour.

@patak-dev
Copy link
Member

About Nuxt, it is also failing in main right now.

@XiSenao
Copy link
Collaborator Author

XiSenao commented Apr 18, 2024 •
edited

I think the behavior of rollup may be considering compatibility with Node (esbuild and webpack also do similar things). If we follow Babel's explanation, for the current scenario (where __esModule is true and the default export is not provided), I think it would be good to provide an error similar to "SyntaxError: The requested module 'xxx' does not provide an export named 'default'". I may lean more towards the latter, but it seems that there is no standardization for __esModule in the community and namespace import may not be friendly, which still appears chaotic to me 😂.

@sapphi-red
Copy link
Member

Given that we cannot support both Node's way and Babel's way, I think it's a good middle-ground. It seems it was discussed at rollup/plugins#537.

bluwy
bluwy reviewed May 2, 2024
View reviewed changes
packages/vite/src/node/plugins/importAnalysis.ts
@@ -845,7 +845,7 @@ export function createParseErrorInfo(
}
}
// prettier-ignore
const interopHelper = (m: any) => m?.__esModule ? m : { ...(typeof m === 'object' && !Array.isArray(m) || typeof m === 'function' ? m : {}), default: m }
const interopHelper = (m: any) => m?.__esModule && Object.prototype.hasOwnProperty.call(m, 'default') ? m : { ...(typeof m === 'object' && !Array.isArray(m) || typeof m === 'function' ? m : {}), default: m }
Copy link
Member

Choose a reason for hiding this comment

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

Let's go ahead with this then to keep dev/build consistent for now. I think we should also add a TODO comment here to revisit this later in Vite 6.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bluwy bluwy bluwy left review comments

@sapphi-red sapphi-red Awaiting requested review from sapphi-red

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
feat: deps optimizer Esbuild Dependencies Optimization has workaround inconsistency Inconsistency between dev & build p2-edge-case Bug, but has workaround or limited in scope (priority)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Vite dev server is unable to import @babel/standalone
4 participants
@XiSenao @sapphi-red @bluwy @patak-dev