fix: preserve default export from externalized packages (fixes #10258) #10406

sapphi-red · 2022-10-10T11:21:59Z

Description

default export was removed because export * from 'foo' won't export "default export".

Changed esbuildCjsExternalPlugin to be only used for require and fixed it to include default export.

fixes #10258
refs #8679

Additional context

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.

sapphi-red · 2022-10-10T11:25:13Z

packages/vite/src/node/optimizer/esbuildDepPlugin.ts


-      build.onLoad({ filter: /.*/, namespace: 'external' }, (args) => ({
-        contents: `export * from ${JSON.stringify(args.path)}`


The first way I came up with is to change this code to:

`export * from ${JSON.stringify(args.path)}` + `export { default } from ${JSON.stringify(args.path)}`

Then, the generated code will be like:

// external:react var react_exports = {}; __export(react_exports, { default: () => default2 }); import { default as default2 } from "react"; import * as react_star from "react"; var init_react = __esm({ "external:react"() { __reExport(react_exports, react_star); } });

This code requires the externalized package (react in this example) to have a default export. But we don't know whether the package has a default export. So this code will fail for a package that doesn't have a default export and cannot be used.

@sapphi-red @bluwy what about

`import mod from ${JSON.stringify(args.path)}` + `module.exports = mod`

seems esbuild handles mixed ESM and CJS. And this fixes the problem with default exports that I mentioned below and ends up creating a smaller bundle for me as well.

That's not correct on two points.

mod is the value of default export. So named exports will be lost.

That import is a self referential. It won't work.

@sapphi-red

mod is the value of default export. So named exports will be lost.

isn't it the default that is returned when you do require('module') which we are targeting with this, is there a way to require named exports ? or asked differently will it be ever an issue that named exports are lost ?

That import is a self referential. It won't work.

not sure about this one, if it is self referential this should not work right ? strangely with this change applied, vite was building properly

isn't it the default that is returned when you do require('module') which we are targeting with this, is there a way to require named exports ?

In node.js, we can't require a ESM file. In webpack, require('./esm.js') returns the module namespace (Module {__esModule: true, Symbol(Symbol.toStringTag): 'Module', default: 'default', named: 'named'}).

not sure about this one, if it is self referential this should not work right ? strangely with this change applied, vite was building properly

I don't know because what you changed is ambiguous. I understood that you changed contents: `export * from ${JSON.stringify(args.path)}` into that. I rethink that it could work.

the patch I had on 3.1.7 is

diff --git a/node_modules/vite/dist/node/chunks/dep-0ebb5606.js b/node_modules/vite/dist/node/chunks/dep-0ebb5606.js index e51e428..35785cd 100644 --- a/node_modules/vite/dist/node/chunks/dep-0ebb5606.js +++ b/node_modules/vite/dist/node/chunks/dep-0ebb5606.js @@ -35222,7 +35222,7 @@ function esbuildCjsExternalPlugin(externals) { namespace: 'external' })); build.onLoad({ filter: /.*/, namespace: 'external' }, (args) => ({ - contents: `export * from ${JSON.stringify(args.path)}` + contents: `import mod from ${JSON.stringify(args.path)}; module.exports = mod` })); } };

but as I said my case was using ssr to build lambda functions only externalising node builtins, so I won't be surprised, it this only works for this very special case 🙈

jiby-aurum · 2022-10-13T09:02:36Z

@sapphi-red @bluwy would it be possible to make it work, when the externalised package is being used in a non-externalised CommonJS package ?

I have this issue with gremlin dependency on my project, I am bundling it in, externalising just the node built-ins. It fails with const EventEmitter = require('events'). Seemingly I have the default on events_exports, but when seemingly __toCommonJS(events_exports) does not use the default as the container for exports. Can reproduce this issue with https://github.com/jiby-aurum/vite-issue by doing npm i && npx vite build && node dist/index.mjs

jiby-aurum · 2022-10-13T13:49:52Z

well seemingly is an esbuild issue (evanw/esbuild#1148) sadly, not sure if it is possible to somehow work around this in vite

sapphi-red · 2022-10-14T11:50:20Z

@jiby-aurum I think that's a different issue. It seems the reason is that Node.js's events module has a different interface between ESM and CJS.

// ESM
const EventEmitter = require('events');
// CJS
import { EventEmitter } from 'events';

jiby-aurum · 2022-10-14T12:10:05Z

@sapphi-red okay makes sense 👍. I just made the remark because with that change, it did fix my particular issue. But my case is a special one, where I am using ssr and externalising just the node builtins. So most probably my use case was not touching any of the cases that this change would break.

btw not sure if the interface is different as these works as well

const { EventEmitter } = require('events')
import EventEmitter from 'events'

bluwy · 2022-11-09T01:23:50Z

Looks like this would need a rebase

…#10258)

sapphi-red · 2022-11-09T11:14:50Z

packages/vite/src/node/optimizer/esbuildDepPlugin.ts

+            `import * as m from ${JSON.stringify(
+              nonFacadePrefix + args.path
+            )};` +
+            `export default m.default;` +
+            `export * from ${JSON.stringify(nonFacadePrefix + args.path)};`


This part is not correct and is not possible to do it in a completely correct way.

In Node environment, it is possible by using const require = createRequire(import.meta.url). I guess this will cover @jiby-aurum's case.
For browsers, I'm not sure how we should handle this one.

I haven't thought of this, but yes. In ssr mode we should avoid rewriting require to import for node built-in modules, but rather make require available via createRequire.

I'm not sure what's not correct here, except that we always return a default export regardless if it has or not? which I think is a good tradeoff for now to get most things working.

I'm not sure what's not correct here

const foo = require('foo') will be converted to import * as foo from 'foo'. But this won't work for every package. That said, I believe it's ok since externalizing required dependencies aren't recommended.

sapphi-red · 2022-11-22T15:18:12Z

packages/vite/src/node/optimizer/esbuildDepPlugin.ts

+          contents:
+            `import * as m from ${JSON.stringify(
+              nonFacadePrefix + args.path
+            )};` + `module.exports = m;`


I changed to module.exports from export * + export { default }.

This will reduce one conversion.

// --- transform from // foo module.exports = 'foo' // bar const foo = require('foo') // --- transform to (previously) // foo:facade export * from 'foo' export { default } from 'foo' // bar const foo = require('foo') // --- transform to // foo:facade import * as m from 'foo' module.exports = m // bar const foo = require('foo')

I guess it's worth trying this out if it works 👍

@bluwy @sapphi-red have been using a similar patch in a medium sized project for a while now without issues, only difference is that I am using the below instead.

import m from ${JSON.stringify( nonFacadePrefix + args.path )};` + `module.exports = m;`

@jiby-aurum Your case would be covered by #11057.

@sapphi-red wow didn't see that one, thanks !!

sapphi-red · 2022-11-22T15:21:54Z

playground/external/dep-that-requires/index.js

@@ -0,0 +1,7 @@
+const { version } = require('vue')
+// require('slash5') // cannot require ESM


Removed this since requiring ESM is not guaranteed to work.

…#10258) (vitejs#10406) fixes vitejs#10258

sapphi-red added p3-minor-bug An edge case that only affects very specific usage (priority) feat: deps optimizer Esbuild Dependencies Optimization labels Oct 10, 2022

sapphi-red commented Oct 10, 2022

View reviewed changes

bluwy previously approved these changes Oct 11, 2022

View reviewed changes

jiby-aurum mentioned this pull request Oct 13, 2022

fix(ssr): change CommonJS and resolve plugin order #10450

Closed

9 tasks

hieunhit mentioned this pull request Oct 24, 2022

vite: vue is undefined in runtime nuxt/bridge#583

Closed

xiaoiver mentioned this pull request Nov 8, 2022

Error: 'EventEmitter' is not exported by node_modules/eventemitter3/index.js antvis/G#1186

Closed

bluwy added the needs rebase label Nov 9, 2022

fix: preserve default export from externalized packages (fixes vitejs…

b1307d8

…#10258)

sapphi-red dismissed bluwy’s stale review via b1307d8 November 9, 2022 10:52

sapphi-red force-pushed the fix/external-default-export branch from 0bca55c to b1307d8 Compare November 9, 2022 10:52

sapphi-red removed the needs rebase label Nov 9, 2022

sapphi-red requested a review from bluwy November 9, 2022 11:11

sapphi-red commented Nov 9, 2022

View reviewed changes

sapphi-red added 5 commits November 22, 2022 22:26

chore: merge main

0fe3bd1

test: don't ensure requiring ESM deps

01eec6a

refactor: use module.exports

e583cf1

test: add test for CJS dep

07dcb79

test: update case

b119433

sapphi-red commented Nov 22, 2022

View reviewed changes

bluwy approved these changes Nov 24, 2022

View reviewed changes

sapphi-red mentioned this pull request Nov 24, 2022

fix(ssr): preserve require for external node #11057

Merged

9 tasks

patak-dev merged commit 88b001b into vitejs:main Nov 24, 2022

sapphi-red deleted the fix/external-default-export branch November 24, 2022 14:38

futurGH pushed a commit to futurGH/vite that referenced this pull request Feb 26, 2023

fix: preserve default export from externalized packages (fixes vitejs…

7fa6762

…#10258) (vitejs#10406) fixes vitejs#10258

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: preserve default export from externalized packages (fixes #10258) #10406

fix: preserve default export from externalized packages (fixes #10258) #10406

sapphi-red commented Oct 10, 2022

sapphi-red Oct 10, 2022 •

edited

Loading

jiby-aurum Oct 13, 2022 •

edited

Loading

sapphi-red Oct 14, 2022

jiby-aurum Oct 14, 2022 •

edited

Loading

sapphi-red Oct 14, 2022

jiby-aurum Oct 14, 2022

jiby-aurum commented Oct 13, 2022

jiby-aurum commented Oct 13, 2022 •

edited

Loading

sapphi-red commented Oct 14, 2022

jiby-aurum commented Oct 14, 2022 •

edited

Loading

bluwy commented Nov 9, 2022

sapphi-red Nov 9, 2022 •

edited

Loading

jiby-aurum Nov 14, 2022

bluwy Nov 17, 2022

sapphi-red Nov 22, 2022

sapphi-red Nov 22, 2022

bluwy Nov 24, 2022

jiby-aurum Nov 24, 2022

sapphi-red Nov 24, 2022

jiby-aurum Nov 24, 2022

sapphi-red Nov 22, 2022


		build.onLoad({ filter: /.*/, namespace: 'external' }, (args) => ({
		contents: `export * from ${JSON.stringify(args.path)}`

		@@ -0,0 +1,7 @@
		const { version } = require('vue')
		// require('slash5') // cannot require ESM

fix: preserve default export from externalized packages (fixes #10258) #10406

fix: preserve default export from externalized packages (fixes #10258) #10406

Conversation

sapphi-red commented Oct 10, 2022

Description

Additional context

What is the purpose of this pull request?

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

sapphi-red Oct 10, 2022 • edited Loading

Choose a reason for hiding this comment

jiby-aurum Oct 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jiby-aurum Oct 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jiby-aurum commented Oct 13, 2022

jiby-aurum commented Oct 13, 2022 • edited Loading

sapphi-red commented Oct 14, 2022

jiby-aurum commented Oct 14, 2022 • edited Loading

bluwy commented Nov 9, 2022

sapphi-red Nov 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sapphi-red Oct 10, 2022 •

edited

Loading

jiby-aurum Oct 13, 2022 •

edited

Loading

jiby-aurum Oct 14, 2022 •

edited

Loading

jiby-aurum commented Oct 13, 2022 •

edited

Loading

jiby-aurum commented Oct 14, 2022 •

edited

Loading

sapphi-red Nov 9, 2022 •

edited

Loading