Update Babel config

[XhmikosR]

• remove plugin-proposal-object-rest-spread
• add bugfixes
• exclude: ['transform-typeof-symbol'] did nothing with our config

Closes #31007

[XhmikosR]

@hzoo just so we are safe, LGTY? :)

[hzoo]

Yeah if you want equivalent behavior sure! I just was confused why you're still defining the plugin here since you removed it from the deps but I guess you are relying on preset-env to have the dep.. but after reading the issue, see loose removes the typeof-symbol plugin so it's not needed, and object-rest-spread is included.. However you can't pass every option down to each plugin specifically (only loose in this case).

Apologies Babel is still so confusing! The difference in output is due to how plugins/presets are working so here's a long explanation. Plugins run before presets so in this case, it's running the object rest spread plugin without loose so it uses the _objectSpread helper function. loose makes it use the _extends helper which is an alias for Object.assign. And there's even another option useBuiltIns which simply replaces it for Object.assign if you can assume that it's polyfilled or supported.

My recommendation is to use loose with object-rest-spread. Is there a reason you don't want "babel is extending Object.assign."? It would save more space due to less helpers being needed as you probably don't use it in a way that requires that spec behavior.

Example from CodeSandbox I made for comparing configs:

[XhmikosR]

@hzoo TBH I was just trying to make sure we do things the right way because we haven't touched this stuff for a long time :)

Is there a reason you don't want "babel is extending Object.assign."?

I thought it would make more sense to go with the official way and not the Object.assign way. Other than that I'm not aware of any other reason. Do note that I didn't originally set this up myself.

So, I'm open to suggestions how to make things the right way. If you think it makes sense to keep the plugin listed in devDependencies, I can revert that part. I'm in between myself since I prefer to list everything I need directly. On the other hand, I didn't even know that this was included in preset-env until I played with it.

Thanks for the reply!

[hzoo]

we haven't touched this stuff for a long time :)
I thought it would make more sense to go with the official way and not the Object.assign way

Totally understandable! I don't think the way people use Babel is thinking about it all the time, it's interesting figuring out how to correlate perf/size/convenience/spec.

Looking at https://unpkg.com/browse/[email protected]/dist/js/bootstrap.js, it includes _objectSpread2, ownKeys, _defineProperty because of the spec version of object rest spread (which is fine) but looking at usage you aren't doing anything like using defineProperties (writeable/enumerable) or using getters/setters so the Object.assign version should be fine. But if we want to make sure it behaves the same that can be another test (or unit test would cover it?).

config = {
  ...Default,
  ...config
}

If you think it makes sense to keep the plugin listed in devDependencies
This is more about how npm works (if it's not directly there it uses the one from somewhere else, so either should be ok.

Also another thing is that if you are dropping IE, you can change the output via babel using targets but i'll write more in a bit

[hzoo]

But yeah my recommendation is to do (blog for more context on bugfixes but it should be default in next major):

module.exports = {
  presets: [
    [
      '@babel/preset-env',
      {
        loose: true,
		bugfixes: true,
        modules: false,
      }
    ]
  ]
}

so I would remove that plugin reference of plugins: ['@babel/plugin-proposal-object-rest-spread'] since it's fine to rely on the loose behavior in these simple cases?

[hzoo]

Was checking out https://blog.getbootstrap.com/2020/06/16/bootstrap-5-alpha/ and I was looking at https://github.com/twbs/bootstrap/blob/main/.browserslistrc, and after some rabbit hole.. it seems like maybe Babel has a bug with parsing Android >= 6 for some reason.. but it looks like Android >= 5 (compat-table/compat-table#801) uses Chrome on Android instead of android webview.

So looks like after 4.4.4, there are no more versions and it just matches mobile android (which follows Chrome's versioning)? So I would suggest changing the value to android 81 or using ChromeAndroid or and_chr for Chrome for Android instead.

This way Babel wouldn't run as many plugins and it would save a lot of bytes since IE is dropped

[XhmikosR]

Sorry for the late reply @hzoo!

I just spent a few hours today due to this :/

I will probably make a new issue because I can't figure this out. Basically, the moment I remove Android our BrowserStack tests fail on Edge.

https://github.com/twbs/bootstrap/pull/30986/commits

IIRC the behavior is normal because after Android 6 (?) Android WebView is using Chrome behind the scenes. What I don't get is why babel doesn't transform the other things so that they still work on Edge. But maybe we are missing something else in our test configuration.

Back to this PR, if you could PR against main what you think is the best that would be perfect :) Ideally, we should backport any cleanup on v4-dev too so that there isn't a big diff between the two branches, apart from the browsers we support.

Thanks for the help!

EDIT: another issue I hit was that Edge >= 16.16299 doesn't include Edge 16 for some reason. So currently on main, browserslist targets Edge 17

[hzoo]

Ok that's very weird.. Android 6 shouldn't represent anything based on my research and Edge 16 is a relatively new version with support for es modules at least on the javascript side so it must be failing because of something else.. and it's odd Android somehow makes Edge/Mobile Safari fail (https://github.com/twbs/bootstrap/runs/788403787?check_suite_focus=true) when both of those are already defined in the browserslist.. with TypeError: Function expected, yeah would need to look into why

[XhmikosR]

I went ahead with your suggestion, although I'd prefer it if you were the patch author :)

It does save us a few bytes from the dist files, not sure if our unit tests cover the Object spread cases. @Johann-S might have a better idea about it.

This tackles the issue on main, for v4-dev I'd need to backport it and see if everything works well.

About the Android issue, I'll make a new issue here to track it, and I already have a draft PR where I experiment with modernizing our browserslist config in #30986, but I hit the aforementioned issue, so I put back Android. Also I split some changes to #31124. Chrome Android is/was already included due to last 1 major version:

C:\Users\xmr\Desktop\bootstrap>npx autoprefixer --info
Browsers:
  Chrome for Android: 81
  Firefox for Android: 68
  And_qq: 10.4
  UC for Android: 12.12
  Android: 81
  Baidu: 7.12
  Chrome: 83, 81, 80, 79, 78, 77, 76, 75, 74, 73, 72, 71, 70, 69, 68, 67, 66, 65, 64, 63, 62, 61, 60
  Edge: 83, 81, 80, 79, 18, 17
  Firefox: 77, 76, 75, 74, 73, 72, 71, 70, 69, 68, 67, 66, 65, 64, 63, 62, 61, 60
  iOS: 13.4-13.5, 13.3, 13.2, 13.0-13.1, 12.2-12.4, 12.0-12.1, 11.3-11.4, 11.0-11.2, 10.3, 10.0-10.2
  Kaios: 2.5
  Opera Mini: all
  Opera Mobile: 46
  Opera: 68
  Safari: 13.1, 13, 12.1, 12, 11.1, 11, 10.1, 10
  Samsung: 11.1-11.2

[hzoo]

Cool this looks good and should be compatible with v4 to backport.

The only breaking change would be modifying the browserslist (dropping IE, but I would try changing Android 6 to Android 81, the difference might be an issue with Babel parsing the browserslist but that should be the right workaround), dropping IE should save a lot more bytes (ex: class A {}).

[XhmikosR]

Yeah, for v5 we have dropped IE. I still want to get the Android issue sorted so I'll make an issue for that.

v4 still supports IE so that doesn't apply there :)