-
-
Notifications
You must be signed in to change notification settings - Fork 198
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
Don't make @babel/preset-env force all transforms in prod #612
Conversation
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.
A small comment, I think it can be good to notice it
👍 Nice! This will definitely be helpful. Thanks. |
bca161d
to
b2809f5
Compare
Oh my, I LOVE this. Encore exists in large part to smooth out the edges of the Webpack ecosystem for our users. However, whenever Webpack makes something better and we can remove hacks, it's wonderful. Thanks @Lyrkan! |
…d (Lyrkan) This PR was merged into the master branch. Discussion ---------- Don't make @babel/preset-env force all transforms in prod This PR removes the `forceAllTransforms: webpackConfig.isProduction()` line that is currently added to `@babel/preset-env`'s options. That setting was previously needed because UglifyJS did not support ES6 which meant that if you, for instance, had `async` functions in your code and targeted a recent browsers, they would have to be transformed for the minification process to be successful. Nowadays we're using Terser which supports ES6, so there shouldn't be any issue keeping these kind of things in the final code anymore. Commits ------- b2809f5 Don't make @babel/preset-env force all transforms in prod
🤔 I've got a problem with that "fix". For instance, my code contains "arrow functions", and my users have IE11. My "browserslistrc" If I add back the line removed here, everything works fine. It's a breakable change that have very bad consequences ... I can technically add in my webpack.config.js the |
Hi @damienfa,
That was the case before that PR since Babel was applying additional transforms for production builds. Now I did a quick test and cannot reproduce your issue locally with the latest version of Encore. Using the following JS file: const foo = () => {
console.log('test');
};
foo();
foo(); And the following {
"devDependencies": {
"@symfony/webpack-encore": "^0.29"
},
"browserslist": [
"last 2 Chrome versions"
]
} I get this output: // $ yarn encore dev
(window["webpackJsonp"] = window["webpackJsonp"] || []).push([["main"],{
/***/ "./src/index.js":
/*!**********************!*\
!*** ./src/index.js ***!
\**********************/
/*! no static exports found */
/***/ (function(module, exports) {
const foo = () => {
console.log('test');
};
foo();
foo();
/***/ })
// $ yarn encore prod
(window.webpackJsonp=window.webpackJsonp||[]).push([["main"],{tjUo:function(o,n){const t=()=>{console.log("test")};t(),t()}},[["tjUo","runtime"]]]); Then if I change the {
"devDependencies": {
"@symfony/webpack-encore": "^0.29"
},
"browserslist": [
"last 2 Chrome versions",
"ie 11"
]
} And I clear babel-loader's cache (required for dev builds, see this page) I get: // $ yarn encore dev
(window["webpackJsonp"] = window["webpackJsonp"] || []).push([["main"],{
/***/ "./src/index.js":
/*!**********************!*\
!*** ./src/index.js ***!
\**********************/
/*! no static exports found */
/***/ (function(module, exports) {
var foo = function foo() {
console.log('test');
};
foo();
foo();
/***/ })
},[["./src/index.js","runtime"]]]);
// $ yarn encore prod
(window.webpackJsonp=window.webpackJsonp||[]).push([["main"],{tjUo:function(n,o){var t=function(){console.log("test")};t(),t()}},[["tjUo","runtime"]]]); |
OK. 🤔 You're right, I've checked with your test code in a brand new webpack project and I cannot reproduce my issue. When I try with the same test code in my first environnement, it still fails : Moreover, I would want to add those 2 points :
Thanks, |
I didn't said it removed things before, just that it didn't apply the same transforms in dev and prod. It basically added things that were not necessarily needed in prod builds in order to make UglifyJS (used before Terser) work properly. That was not nominal since some optimizations can rely on recent ES features that were "fixed" even though your browserslist allowed them (for instance using arrow functions instead of standard functions).
I didn't do anything special related to Babel in my const Encore = require('@symfony/webpack-encore');
Encore
.enableSingleRuntimeChunk()
.cleanupOutputBeforeBuild()
.setOutputPath('build/')
.setPublicPath('/')
.cleanupOutputBeforeBuild()
.addEntry('main', './src/index.js')
.enableVersioning(Encore.isProduction())
;
module.exports = Encore.getWebpackConfig(); You shouldn't have to set |
This PR removes the
forceAllTransforms: webpackConfig.isProduction()
line that is currently added to@babel/preset-env
's options.That setting was previously needed because UglifyJS did not support ES6 which meant that if you, for instance, had
async
functions in your code and targeted a recent browsers, they would have to be transformed for the minification process to be successful.Nowadays we're using Terser which supports ES6, so there shouldn't be any issue keeping these kind of things in the final code anymore.