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

deprecates babel/polyfill in favor of core-js@3 #2031

Merged
merged 1 commit into from May 28, 2019
Merged

deprecates babel/polyfill in favor of core-js@3 #2031

merged 1 commit into from May 28, 2019

Conversation

somebody32
Copy link
Contributor

@somebody32 somebody32 commented Mar 26, 2019

babel/polyfill is deprecated and using core-js directly is the way now.

This pr adds the required modifications to support core-js@3.
I didn't remove babel/polyfill from the dependencies to not make this change breaking.

Update: removed babel/polyfill because updating core-js would break old usage anyway, so updating after that PR would require changes to the user's codebase

@@ -6,7 +6,7 @@ const { nodeEnv } = require('../env')
module.exports = {
test: /\.(js|mjs)$/,
include: /node_modules/,
exclude: /@babel(?:\/|\\{1,2})runtime/,
exclude: /@babel(?:\/|\\{1,2})runtime|core-js/,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

running babel on core-js is causing issues

@renchap
Copy link
Contributor

renchap commented Apr 1, 2019

#2031 (comment) : this is important, as this breaks IE11 for any person polyfilling Symbol and using core-js@3.

@jakeNiemiec
Copy link
Member

Babel PR: babel/babel#7646

package.json Outdated
"@babel/plugin-transform-destructuring": "^7.4.0",
"@babel/plugin-transform-regenerator": "^7.4.0",
"@babel/plugin-transform-runtime": "^7.4.0",
"@babel/polyfill": "^7.4.0",
Copy link
Member

Choose a reason for hiding this comment

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

Is this needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry, needed what? @babel/polyfill? yes, if we remove it, then it is going to be a breaking change

Choose a reason for hiding this comment

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

(Disclaimer: I know Babel and core-js pretty well, but not webpacker)

Why would it be a breaking change? Is it exposed to your users or is it only added by webpacker?

Copy link
Member

Choose a reason for hiding this comment

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

image
@somebody32 The document you linked seems to indicate that it needs to be removed. Apart from that, I feel like this is defenitly a breaking change since users will need to change their pack files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nicolo-ribaudo @jakeNiemiec you're right, I for some reason thought that core-js@3 and @babel/polyfill could coexist for some time, but looks like it is not the cause. My idea initially was not to remove @babel/polyfill, so builds which indirectly dependant on it will not be broken, but looks like it is safer to remove it so there will be no strange compatibility problems. I'll change that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@connorshea
Copy link
Contributor

We should maybe have a changelog entry written up that has an upgrade guide.

@@ -30,6 +30,7 @@ module.exports = function(api) {
{
forceAllTransforms: true,
useBuiltIns: 'entry',
corejs: 3,
modules: false,
exclude: ['transform-typeof-symbol']
}
Copy link
Contributor

Choose a reason for hiding this comment

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

@somebody32 This suggests that @babel/plugin-transform-runtime (line 56) can specify to use corejs: 3 as dependency. Were you able to make that work? I'm still seeing errors like these when I make that change:

Screen Shot 2019-04-18 at 11 11 44 PM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

great point, looks like I need to extend that too (was not using it in our project, that is why didn't spot the problem): https://github.com/zloirock/core-js/blob/master/docs/2019-03-19-core-js-3-babel-and-a-look-into-the-future.md#babelruntime

tldr: try to add @babel/runtime-corejs3 package

@@ -30,6 +30,7 @@ module.exports = function(api) {
{
forceAllTransforms: true,
useBuiltIns: 'entry',
Copy link
Contributor

Choose a reason for hiding this comment

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

@gauravtiwari do you think usage should be the new default? From here:

Until Babel 7.3, useBuiltIns: usage was unstable and not fully reliable: many polyfills were not included, and many others were added without their required dependencies. In Babel 7.4, I tried to make it understand every possible usage pattern.

it does look like it's stable now. If I understand it correctly, using usage won't require additional import. When I use:

import "core-js";

in my entry point, I get the following message:

When setting `useBuiltIns: 'usage'`, polyfills are automatically imported when needed.
Please remove the direct import of `core-js` or use `useBuiltIns: 'entry'` instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would question that. useBuiltIns: 'usage' is not assuming anything about user's code, just expect to change import core-js to required imports based on supported env, so you need to opt-in in into it by adding that import into one of your packs.
If we switch that to usage, it stops being opt-in and starts automatically adding imports to the code. And from my experience, it still has some edge-cases (because core-js detects relevant import by statically analyzing, so it could not predict what you could have in a runtime), so that creates cases when you still need to add some polyfills by hand.

To summarize: I think useBuiltIns: 'usage' is a safer choice, in general, that is very easy to change if you know what you're doing.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense 👍

@somebody32
Copy link
Contributor Author

To be honest, I'm not sure what happened to the specs, looks like a configuration issue, and I believe master branch is failing too

docs/es6.md Outdated

Don't forget to import `@babel/polyfill` in your main entry point like so:
Don't forget add these lines into your main entry point:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Don't forget add these lines into your main entry point:
Don't forget to add these lines into your main entry point:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, thanks!

@PikachuEXE
Copy link

Any more action is required for this PR?

@somebody32
Copy link
Contributor Author

@PikachuEXE reviewing and merging, I believe

@gauravtiwari
Copy link
Member

Thanks everyone, will review and merge this over the weekend and make a new release.

@jaredbeck
Copy link
Contributor

A recent version of babel (perhaps 7.4.5?) now produces a warning about explicitly setting the core-js version.

WARNING: We noticed you're using the useBuiltIns option without declaring a core-js version. Currently, we assume version 2.x when no version is passed. Since this default version will likely change in future versions of Babel, we recommend explicitly setting the core-js version you are using via the corejs option...

For now, I'm using the following patch ..

       (isProductionEnv || isDevelopmentEnv) && [
         require('@babel/preset-env').default,
         {
           forceAllTransforms: true,
           useBuiltIns: 'entry',
+          corejs: '2',

Does this PR update need to update the webpacker:install rake task accordingly? Sorry if it does already and I'm just not seeing it.

@somebody32
Copy link
Contributor Author

@jaredbeck it is here: https://github.com/rails/webpacker/pull/2031/files#diff-13f93725bce9fb269a61b40f3f4f10deR33

@PikachuEXE
Copy link

@gauravtiwari Any plan to review this PR soon?

@jakeNiemiec
Copy link
Member

Made a PR where I added the upgrade instructions to the changelog, feedback appreciated: #2107

Are there any other outstanding issues with core-js + babel?

gauravtiwari pushed a commit that referenced this pull request May 28, 2019
Taking @connorshea's suggestion from #2031 (comment) to warn users who were using `@babel/polyfill`
@BobbyMcWho
Copy link

This change breaks any project using tailwindcss just as an FYI.

$ bin/webpack-dev-server
ℹ 「wds」: Project is running at http://localhost:3035/
ℹ 「wds」: webpack output is served from /packs/
ℹ 「wds」: Content not from webpack is served from /mnt/d/_repos/test-tailwinds-css/public/packs
ℹ 「wds」: 404s will fallback to /index.html
✖ 「wdm」: Hash: c31b9d5f78833e23efd1
Version: webpack 4.32.2
Time: 4254ms
Built at: 05/29/2019 9:54:57 AM
                                     Asset       Size       Chunks             Chunk Names
    js/application-d806edd0e75361e03489.js    646 KiB  application  [emitted]  application
js/application-d806edd0e75361e03489.js.map    563 KiB  application  [emitted]  application
                             manifest.json  364 bytes               [emitted]

ERROR in ./app/javascript/css/application.css
Module build failed (from ./node_modules/mini-css-extract-plugin/dist/loader.js):
TypeError: __webpack_require__(...) is not a function
    at Module../node_modules/webpack/buildin/harmony-module.js (/mnt/d/_repos/test-tailwinds-css/node_modules/css-loader/dist/cjs.js??ref--6-1!/mnt/d/_repos/test-tailwinds-css/node_modules/postcss-loader/src/index.js??ref--6-2!/mnt/d/_repos/test-tailwinds-css/app/javascript/css/application.css:3369:139)
    at __webpack_require__ (/mnt/d/_repos/test-tailwinds-css/node_modules/css-loader/dist/cjs.js??ref--6-1!/mnt/d/_repos/test-tailwinds-css/node_modules/postcss-loader/src/index.js??ref--6-2!/mnt/d/_repos/test-tailwinds-css/app/javascript/css/application.css:21:30)
    at Module../node_modules/css-loader/dist/runtime/api.js (/mnt/d/_repos/test-tailwinds-css/node_modules/css-loader/dist/cjs.js??ref--6-1!/mnt/d/_repos/test-tailwinds-css/node_modules/postcss-loader/src/index.js??ref--6-2!/mnt/d/_repos/test-tailwinds-css/app/javascript/css/application.css:3279:41)
    at __webpack_require__ (/mnt/d/_repos/test-tailwinds-css/node_modules/css-loader/dist/cjs.js??ref--6-1!/mnt/d/_repos/test-tailwinds-css/node_modules/postcss-loader/src/index.js??ref--6-2!/mnt/d/_repos/test-tailwinds-css/app/javascript/css/application.css:21:30)
    at Object../node_modules/css-loader/dist/cjs.js?!./node_modules/postcss-loader/src/index.js?!./app/javascript/css/application.css (/mnt/d/_repos/test-tailwinds-css/node_modules/css-loader/dist/cjs.js??ref--6-1!/mnt/d/_repos/test-tailwinds-css/node_modules/postcss-loader/src/index.js??ref--6-2!/mnt/d/_repos/test-tailwinds-css/app/javascript/css/application.css:3161:28)
    at __webpack_require__ (/mnt/d/_repos/test-tailwinds-css/node_modules/css-loader/dist/cjs.js??ref--6-1!/mnt/d/_repos/test-tailwinds-css/node_modules/postcss-loader/src/index.js??ref--6-2!/mnt/d/_repos/test-tailwinds-css/app/javascript/css/application.css:21:30)
    at /mnt/d/_repos/test-tailwinds-css/node_modules/css-loader/dist/cjs.js??ref--6-1!/mnt/d/_repos/test-tailwinds-css/node_modules/postcss-loader/src/index.js??ref--6-2!/mnt/d/_repos/test-tailwinds-css/app/javascript/css/application.css:85:18
    at Object.<anonymous> (/mnt/d/_repos/test-tailwinds-css/node_modules/css-loader/dist/cjs.js??ref--6-1!/mnt/d/_repos/test-tailwinds-css/node_modules/postcss-loader/src/index.js??ref--6-2!/mnt/d/_repos/test-tailwinds-css/app/javascript/css/application.css:88:10)
    at Module._compile (module.js:652:30)
    at exec (/mnt/d/_repos/test-tailwinds-css/node_modules/mini-css-extract-plugin/dist/loader.js:58:10)
ℹ 「wdm」: Failed to compile.

For anyone else who manages to find their way here, you have to remove the corejs: 3 line from your babel.config.js to get successful asset compilation again.

@jakeNiemiec
Copy link
Member

@BobbyMcWho Thanks for pointing this out. Did you remember to run bundle exec rails webpacker:install since this added new packages? Did you set up webpacker to use tailwind with postcss?

@BobbyMcWho
Copy link

Yeah, I double checked all the config files, the only thing I could get to fix it was to remove the corejs: 3 line from the babel.config.js, and then it compiled just fine afterward.

I can try and put together a reference app when I have a bit of spare time.

@jakeNiemiec
Copy link
Member

@BobbyMcWho
Copy link

BobbyMcWho commented May 29, 2019

So the issue happens when the assets are compiled using webpack as part of bundle exec rails s on page load, it just displays in the browser console instead. I have figured out that the only place that I need to set corejs: 2 is in the following section of the babel.config.js:

      [
        require('@babel/plugin-transform-runtime').default,
        {
          helpers: false,
          regenerator: true,
          corejs: 2
        }
      ],

This lets tailwindcss styles get compiled just fine.

Edit: Apparently I have to completely remove corejs in the above to get it to work, forgot to save after adding corejs: 2

@jakeNiemiec
Copy link
Member

I have created a PR to fix this here (works with corejs: 3): #2110

The answer to the problem was that things break when you transpile anything that has to do with transpiling. Picture your modules doing something like this (but poorly):

image

@gauravtiwari
Copy link
Member

@jakeNiemiec Perhaps, we remove the corejs option in runtime plugin. Sort of confusing that we have env preset that should take care of everything with corejs enabled but we have similar options available for other plugins. Did you find any difference, if we remove corejs option from runtime plugin?

jakeNiemiec added a commit that referenced this pull request Jun 3, 2019
- Prevents `@babel/plugin-transform-runtime` from rewriting babel helpers in core-js.
- Remove unneeded runtime `@babel/runtime-corejs3`

cc: #2031, #2109
@jakeNiemiec
Copy link
Member

jakeNiemiec commented Jun 3, 2019

Perhaps, we remove the corejs option in runtime plugin.

Fully agree, plus create-react-app followed that same path.

Did you find any difference, if we remove corejs option from runtime plugin?

Setting it to false seems to fix things for quite a few users. I made a PR that implements and documents this fix: #2116

gauravtiwari pushed a commit that referenced this pull request Jun 3, 2019
- Prevents `@babel/plugin-transform-runtime` from rewriting babel helpers in core-js.
- Remove unneeded runtime `@babel/runtime-corejs3`

cc: #2031, #2109
@somebody32 somebody32 deleted the feature/migrate-to-core-js-3 branch June 11, 2019 07:47
KingTiger001 added a commit to KingTiger001/Rails-web-pack-project that referenced this pull request Jan 15, 2023
Taking @connorshea's suggestion from rails/webpacker#2031 (comment) to warn users who were using `@babel/polyfill`
KingTiger001 added a commit to KingTiger001/Rails-web-pack-project that referenced this pull request Jan 15, 2023
- Prevents `@babel/plugin-transform-runtime` from rewriting babel helpers in core-js.
- Remove unneeded runtime `@babel/runtime-corejs3`

cc: rails/webpacker#2031, rails/webpacker#2109
smartech7 pushed a commit to smartech7/ruby-webpacker that referenced this pull request Aug 4, 2023
Taking @connorshea's suggestion from rails/webpacker#2031 (comment) to warn users who were using `@babel/polyfill`
smartech7 pushed a commit to smartech7/ruby-webpacker that referenced this pull request Aug 4, 2023
- Prevents `@babel/plugin-transform-runtime` from rewriting babel helpers in core-js.
- Remove unneeded runtime `@babel/runtime-corejs3`

cc: rails/webpacker#2031, rails/webpacker#2109
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet