-
-
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 all commits
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 |
---|---|---|
|
@@ -16,6 +16,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 | ||
|
@@ -268,19 +271,36 @@ 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 | ||
)};` + `module.exports = m;` | ||
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 changed to 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') 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 guess it's worth trying this out if it works 👍 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. @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.
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. @jiby-aurum Your case would be covered by #11057. 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. @sapphi-red wow didn't see that one, thanks !! |
||
}) | ||
) | ||
} | ||
} | ||
} |
This file was deleted.
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
import { version } from 'vue' | ||
import slash5 from 'slash5' | ||
import slash3 from 'slash3' | ||
|
||
document.querySelector('#imported-vue-version').textContent = version | ||
document.querySelector('#imported-slash5-exists').textContent = | ||
!!slash5('foo/bar') | ||
document.querySelector('#imported-slash3-exists').textContent = | ||
!!slash3('foo/bar') |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
{ | ||
"name": "@vitejs/dep-that-imports", | ||
"private": true, | ||
"version": "0.0.0", | ||
"dependencies": { | ||
"slash3": "npm:slash@^3.0.0", | ||
"slash5": "npm:slash@^5.0.0", | ||
"vue": "^3.2.45" | ||
} | ||
} |
This file was deleted.
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
const { version } = require('vue') | ||
// require('slash5') // cannot require ESM | ||
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. Removed this since requiring ESM is not guaranteed to work. |
||
const slash3 = require('slash3') | ||
|
||
document.querySelector('#required-vue-version').textContent = version | ||
document.querySelector('#required-slash3-exists').textContent = | ||
!!slash3('foo/bar') |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
{ | ||
"name": "@vitejs/dep-that-requires", | ||
"private": true, | ||
"version": "0.0.0", | ||
"dependencies": { | ||
"slash3": "npm:slash@^3.0.0", | ||
"slash5": "npm:slash@^5.0.0", | ||
"vue": "^3.2.45" | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,14 +7,19 @@ | |
<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", | ||
"slash5": "https://unpkg.com/[email protected]/index.js", | ||
"slash3": "https://esm.sh/[email protected]" | ||
} | ||
} | ||
</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 slash5 exists: <span id="imported-slash5-exists"></span></p> | ||
<p>Imported slash3 exists: <span id="imported-slash3-exists"></span></p> | ||
<p>Required slash3 exists: <span id="required-slash3-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', 'slash5'] | ||
}, | ||
build: { | ||
minify: false, | ||
rollupOptions: { | ||
external: ['vue'] | ||
external: ['vue', 'slash3', 'slash5'] | ||
}, | ||
commonjsOptions: { | ||
esmExternals: ['vue'] | ||
esmExternals: ['vue', 'slash5'] | ||
} | ||
} | ||
}) |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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 🙈