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

Migrate from @babel/polyfill to core-js@3 #19872

Merged
merged 1 commit into from
Mar 26, 2020

Conversation

ChristophWurst
Copy link
Member

@ChristophWurst
Copy link
Member Author

FYI the main.js bundle shrinks from 1328kB to 1240kB = ~7% 🚀

Copy link
Member

@juliusknorr juliusknorr left a comment

Choose a reason for hiding this comment

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

Does not work in IE11 anymore:

image

Mainly it seems that Object.assign is no longer properly polyfilled

@ChristophWurst
Copy link
Member Author

Does not work in IE11 anymore

Huh. Is it because with corejs only the bundled files are polyfilled and anything external isn't? 😢

@juliusknorr
Copy link
Member

Probably, I just checked the login page and it was an Object.assign in backbone.js that caused that.

@ChristophWurst
Copy link
Member Author

Okay. Then we can either

  • Close this PR and proceed with what we had before
  • Change this PR to still ship the full-fledged polyfill but also just inline the corejs usage for bundled files

@juliusknorr
Copy link
Member

Not sure if that fixes it but maybe we could also make sure that the affected node modules are transpiled as well? https://github.com/nextcloud/server/blob/master/webpack.common.js#L86

@ChristophWurst
Copy link
Member Author

It will help with the packages where we know that transpilation is requried. But anything else might break. I think the safe path is to still ship a polyfill.

On that note. We might want to document that apps shouldn't rely on it, so we can phase that out?

@ChristophWurst ChristophWurst added the pending documentation This pull request needs an associated documentation update label Mar 10, 2020
@skjnldsv
Copy link
Member

skjnldsv commented Mar 10, 2020

You also need other stuff to replace polyfill:
https://babeljs.io/docs/en/babel-polyfill

import "core-js/stable";
import "regenerator-runtime/runtime";

@ChristophWurst
Copy link
Member Author

FYI the main.js bundle shrinks from 1328kB to 1240kB = ~7% rocket

Now we're at 1356kB :rocket_crashing:

babel.config.js Outdated Show resolved Hide resolved
@ChristophWurst ChristophWurst force-pushed the techdebt/bable-polyfill-to-core-js branch from f987fb7 to fa9c206 Compare March 26, 2020 12:23
@ChristophWurst
Copy link
Member Author

Mainly it seems that Object.assign is no longer properly polyfilled

Tested again with IE11, Object.assign was there :)

Copy link
Member

@gary-kim gary-kim left a comment

Choose a reason for hiding this comment

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

LGTM. I am a bit worried about this but it looks like other people have write access so it should still be supported.

EDIT: TLDR, the author and primary maintainer of the project has been sentenced to 1.5 years in prison.

@ChristophWurst
Copy link
Member Author

LGTM. I am a bit worried about this but it looks like other people have write access so it should still be supported.

Ouch. Didn't know about that. As the polyfill is deprecated this is our best choice right now. If alternatives evolve, we should be able to migrate to them.

@skjnldsv skjnldsv merged commit 36d76ee into master Mar 26, 2020
@skjnldsv skjnldsv deleted the techdebt/bable-polyfill-to-core-js branch March 26, 2020 15:42
@skjnldsv skjnldsv added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Mar 26, 2020
@ChristophWurst ChristophWurst removed the pending documentation This pull request needs an associated documentation update label Nov 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish technical debt
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants