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.

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

Merged
merged 6 commits into from Nov 24, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
44 changes: 33 additions & 11 deletions packages/vite/src/node/optimizer/esbuildDepPlugin.ts
Expand Up @@ -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
Expand Down Expand Up @@ -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)}`
Copy link
Member Author

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:

`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.

Copy link
Contributor

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

`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.

Copy link
Member Author

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.

  1. mod is the value of default export. So named exports will be lost.
  2. That import is a self referential. It won't work.

Copy link
Contributor

Choose a reason for hiding this comment

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

@sapphi-red

  1. 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 ?

  1. 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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

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

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 🙈

}))
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)};`
Copy link
Member Author

@sapphi-red sapphi-red Nov 9, 2022

Choose a reason for hiding this comment

The 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 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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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 require available via createRequire.

Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

})
)
}
}
}
5 changes: 5 additions & 0 deletions playground/external/__tests__/external.spec.ts
Expand Up @@ -7,6 +7,11 @@ test('importmap', () => {
)
})

test('should have default exports', async () => {
expect(await page.textContent('#imported-slash-exists')).toBe('true')
expect(await page.textContent('#required-slash-exists')).toBe('true')
})

describe.runIf(isBuild)('build', () => {
test('should externalize imported packages', async () => {
// If `vue` is successfully externalized, the page should use the version from the import map
Expand Down
@@ -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
@@ -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"
}
}
3 changes: 0 additions & 3 deletions playground/external/dep-that-requires-vue/index.js

This file was deleted.

5 changes: 5 additions & 0 deletions playground/external/dep-that-requires/index.js
@@ -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
@@ -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"
}
}
5 changes: 4 additions & 1 deletion playground/external/index.html
Expand Up @@ -7,14 +7,17 @@
<script type="importmap">
{
"imports": {
"vue": "https://unpkg.com/vue@3.2.0/dist/vue.runtime.esm-browser.js"
"vue": "https://unpkg.com/vue@3.2.0/dist/vue.runtime.esm-browser.js",
"slash": "https://unpkg.com/slash@5.0.0/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>
5 changes: 3 additions & 2 deletions playground/external/package.json
Expand Up @@ -9,10 +9,11 @@
"preview": "vite preview"
},
"dependencies": {
"@vitejs/dep-that-imports-vue": "file:./dep-that-imports-vue",
"@vitejs/dep-that-requires-vue": "file:./dep-that-requires-vue"
"@vitejs/dep-that-imports": "file:./dep-that-imports",
"@vitejs/dep-that-requires": "file:./dep-that-requires"
},
"devDependencies": {
"slash": "^5.0.0",
"vite": "workspace:*",
"vue": "^3.2.41"
}
Expand Down
4 changes: 2 additions & 2 deletions playground/external/src/main.js
@@ -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'
8 changes: 6 additions & 2 deletions playground/external/vite.config.js
@@ -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']
}
}
})