-
-
Notifications
You must be signed in to change notification settings - Fork 6.4k
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
fix: preserve default export from externalized packages (fixes #10258) #10406
Changes from 1 commit
b1307d8
0fe3bd1
01eec6a
e583cf1
07dcb79
b119433
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,6 +17,9 @@ const externalWithConversionNamespace = | |
'vite:dep-pre-bundle:external-conversion' | ||
const convertedExternalPrefix = 'vite-dep-pre-bundle-external:' | ||
|
||
const cjsExternalFacadeNamespace = 'vite:cjs-external-facade' | ||
const nonFacadePrefix = 'vite-cjs-external-facade:' | ||
|
||
const externalTypes = [ | ||
'css', | ||
// supported pre-processor types | ||
|
@@ -270,19 +273,38 @@ export function esbuildCjsExternalPlugin(externals: string[]): Plugin { | |
`^${text.replace(/[-\/\\^$*+?.()|[\]{}]/g, '\\$&')}$` | ||
const filter = new RegExp(externals.map(escape).join('|')) | ||
|
||
build.onResolve({ filter: /.*/, namespace: 'external' }, (args) => ({ | ||
path: args.path, | ||
external: true | ||
})) | ||
build.onResolve({ filter: new RegExp(`^${nonFacadePrefix}`) }, (args) => { | ||
return { | ||
path: args.path.slice(nonFacadePrefix.length), | ||
external: true | ||
} | ||
}) | ||
|
||
build.onResolve({ filter }, (args) => { | ||
if (args.kind === 'require-call') { | ||
return { | ||
path: args.path, | ||
namespace: cjsExternalFacadeNamespace | ||
} | ||
} | ||
|
||
build.onResolve({ filter }, (args) => ({ | ||
path: args.path, | ||
namespace: 'external' | ||
})) | ||
return { | ||
path: args.path, | ||
external: true | ||
} | ||
}) | ||
|
||
build.onLoad({ filter: /.*/, namespace: 'external' }, (args) => ({ | ||
contents: `export * from ${JSON.stringify(args.path)}` | ||
})) | ||
build.onLoad( | ||
{ filter: /.*/, namespace: cjsExternalFacadeNamespace }, | ||
(args) => ({ | ||
contents: | ||
`import * as m from ${JSON.stringify( | ||
nonFacadePrefix + args.path | ||
)};` + | ||
`export default m.default;` + | ||
`export * from ${JSON.stringify(nonFacadePrefix + args.path)};` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
}) | ||
) | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,5 @@ | ||
import { version } from 'vue' | ||
import slash from 'slash' | ||
|
||
document.querySelector('#imported-vue-version').textContent = version | ||
document.querySelector('#imported-slash-exists').textContent = !!slash |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,8 +1,9 @@ | ||
{ | ||
"name": "@vitejs/dep-that-imports-vue", | ||
"name": "@vitejs/dep-that-imports", | ||
"private": true, | ||
"version": "0.0.0", | ||
"dependencies": { | ||
"slash": "^5.0.0", | ||
"vue": "^3.2.41" | ||
} | ||
} |
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
const { version } = require('vue') | ||
const { default: slash } = require('slash') | ||
|
||
document.querySelector('#required-vue-version').textContent = version | ||
document.querySelector('#required-slash-exists').textContent = !!slash |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,8 +1,9 @@ | ||
{ | ||
"name": "@vitejs/dep-that-requires-vue", | ||
"name": "@vitejs/dep-that-requires", | ||
"private": true, | ||
"version": "0.0.0", | ||
"dependencies": { | ||
"slash": "^5.0.0", | ||
"vue": "^3.2.41" | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,14 +7,17 @@ | |
<script type="importmap"> | ||
{ | ||
"imports": { | ||
"vue": "https://unpkg.com/[email protected]/dist/vue.runtime.esm-browser.js" | ||
"vue": "https://unpkg.com/[email protected]/dist/vue.runtime.esm-browser.js", | ||
"slash": "https://unpkg.com/[email protected]/index.js" | ||
} | ||
} | ||
</script> | ||
</head> | ||
<body> | ||
<p>Imported Vue version: <span id="imported-vue-version"></span></p> | ||
<p>Required Vue version: <span id="required-vue-version"></span></p> | ||
<p>Imported slash exists: <span id="imported-slash-exists"></span></p> | ||
<p>Required slash exists: <span id="required-slash-exists"></span></p> | ||
<script type="module" src="/src/main.js"></script> | ||
</body> | ||
</html> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,2 +1,2 @@ | ||
import '@vitejs/dep-that-imports-vue' | ||
import '@vitejs/dep-that-requires-vue' | ||
import '@vitejs/dep-that-imports' | ||
import '@vitejs/dep-that-requires' |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,13 +1,17 @@ | ||
import { defineConfig } from 'vite' | ||
|
||
export default defineConfig({ | ||
optimizeDeps: { | ||
include: ['dep-that-imports', 'dep-that-requires'], | ||
exclude: ['vue', 'slash'] | ||
}, | ||
build: { | ||
minify: false, | ||
rollupOptions: { | ||
external: ['vue'] | ||
external: ['vue', 'slash'] | ||
}, | ||
commonjsOptions: { | ||
esmExternals: ['vue'] | ||
esmExternals: ['vue', 'slash'] | ||
} | ||
} | ||
}) |
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.
The first way I came up with is to change this code to:
Then, the generated code will be like:
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.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.
@sapphi-red @bluwy what about
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.
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.
That's not correct on two points.
mod
is the value of default export. So named exports will be lost.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.
@sapphi-red
isn't it the default that is returned when you do
require('module')
which we are targeting with this, is there a way torequire
named exports ? or asked differently will it be ever an issue that named exports are lost ?not sure about this one, if it is self referential this should not work right ? strangely with this change applied, vite was building properly
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.
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'}
).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.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.
the patch I had on 3.1.7 is
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 🙈