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 all commits
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
42 changes: 31 additions & 11 deletions packages/vite/src/node/optimizer/esbuildDepPlugin.ts
Expand Up @@ -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
Expand Down Expand Up @@ -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)}`
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
)};` + `module.exports = m;`
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 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')

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

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 wow didn't see that one, thanks !!

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

test('should have default exports', async () => {
expect(await page.textContent('#imported-slash5-exists')).toBe('true')
expect(await page.textContent('#imported-slash3-exists')).toBe('true')
expect(await page.textContent('#required-slash3-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
3 changes: 0 additions & 3 deletions playground/external/dep-that-imports-vue/index.js

This file was deleted.

8 changes: 0 additions & 8 deletions playground/external/dep-that-imports-vue/package.json

This file was deleted.

9 changes: 9 additions & 0 deletions playground/external/dep-that-imports/index.js
@@ -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')
10 changes: 10 additions & 0 deletions playground/external/dep-that-imports/package.json
@@ -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"
}
}
3 changes: 0 additions & 3 deletions playground/external/dep-that-requires-vue/index.js

This file was deleted.

8 changes: 0 additions & 8 deletions playground/external/dep-that-requires-vue/package.json

This file was deleted.

7 changes: 7 additions & 0 deletions playground/external/dep-that-requires/index.js
@@ -0,0 +1,7 @@
const { version } = require('vue')
// require('slash5') // cannot require ESM
Copy link
Member Author

Choose a reason for hiding this comment

The 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')
10 changes: 10 additions & 0 deletions playground/external/dep-that-requires/package.json
@@ -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"
}
}
7 changes: 6 additions & 1 deletion playground/external/index.html
Expand Up @@ -7,14 +7,19 @@
<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",
"slash5": "https://unpkg.com/slash@5.0.0/index.js",
"slash3": "https://esm.sh/slash@3.0.0"
}
}
</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>
6 changes: 4 additions & 2 deletions playground/external/package.json
Expand Up @@ -9,10 +9,12 @@
"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": {
"slash3": "npm:slash@^3.0.0",
"slash5": "npm:slash@^5.0.0",
"vite": "workspace:*",
"vue": "^3.2.45"
}
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', 'slash5']
},
build: {
minify: false,
rollupOptions: {
external: ['vue']
external: ['vue', 'slash3', 'slash5']
},
commonjsOptions: {
esmExternals: ['vue']
esmExternals: ['vue', 'slash5']
}
}
})
43 changes: 28 additions & 15 deletions pnpm-lock.yaml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.