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.

Sign up for GitHub

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

refactor(legacy): build polyfill chunk #9639

Merged
merged 1 commit into from
Aug 19, 2022
Merged

refactor(legacy): build polyfill chunk #9639

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 12 additions & 17 deletions packages/plugin-legacy/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -657,8 +657,6 @@ async function buildPolyfillChunk(
plugins: [polyfillsPlugin(imports, excludeSystemJS)],
build: {
write: false,
// if a value above 'es5' is set, esbuild injects helper functions which uses es2015 features
target: 'es5',
minify,
assetsDir,
rollupOptions: {
Expand All @@ -671,6 +669,18 @@ async function buildPolyfillChunk(
manualChunks: undefined
}
}
},
// Don't run esbuild for transpilation or minification
// because we don't want to transpile code.
esbuild: false,
optimizeDeps: {
esbuildOptions: {
// If a value above 'es5' is set, esbuild injects helper functions which uses es2015 features.
// This limits the input code not to include es2015+ codes.
// But core-js is the only dependency which includes commonjs code
// and core-js doesn't include es2015+ codes.
target: 'es5'
}
}
Comment on lines +673 to 684
Copy link
Member

Choose a reason for hiding this comment

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

Following up of the convo in #9635, I think esbuild: false make sense since you showed that core-js is already in es5, and same for systemjs (except it's now using const).

I don't quite understand the optimizeDeps.esbuildOptions.target part though since optimizeDeps is only for dev only for now, unless you're anticipating when we optimize in build too? I'd actually slightly lean to not optimize in build too then to simplify the bundling process 🤔

Copy link
Member Author

@sapphi-red sapphi-red Aug 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Ah, yes, we could remove this for now.
#8660 was introduced when optimize in build was enabled by default. We'll need to add this or enable commonjs plugin when optimize in build is enabled by default.

Copy link
Member

Choose a reason for hiding this comment

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

Actually thinking now, maybe it wouldn't hurt leaving it in anyways, but it's definitely something to keep in mind when we completely remove the commonjs plugin

})
const _polyfillChunk = Array.isArray(res) ? res[0] : res
Expand Down Expand Up @@ -710,21 +720,6 @@ function polyfillsPlugin(
(excludeSystemJS ? '' : `import "systemjs/dist/s.min.js";`)
)
}
},
renderChunk(_, __, opts) {
// systemjs includes code that can't be minified down to es5 by esbuild
if (!excludeSystemJS) {
// @ts-ignore avoid esbuild transform on legacy chunks since it produces
// legacy-unsafe code - e.g. rewriting object properties into shorthands
opts.__vite_skip_esbuild__ = true

// @ts-ignore force terser for legacy chunks. This only takes effect if
// minification isn't disabled, because that leaves out the terser plugin
// entirely.
opts.__vite_force_terser__ = true
}

return null
}
}
}
Expand Down