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

remove core-js #167

Merged
merged 16 commits into from
Jul 22, 2021
Merged

remove core-js #167

merged 16 commits into from
Jul 22, 2021

Conversation

adi518
Copy link
Contributor

@adi518 adi518 commented Oct 20, 2020

This PR removes core-js (ES polyfills) and leaves it outside the bundle of this package. Consumers should now add polyfills to their app if they need to support old browsers. I wonder whether it should be released as a new major version, WDYT?

Other stuff this PR does:

  • Bumps @babel/cli to latest minor (fixes some issues and adds --no-comments flag).
  • Minify (--minified + --no-comments) which lowers the bundle size even more.
  • Adds source-map generation (let me know if you want to keep it).

Next steps:
Use Webpack 5 or Rollup to build an ESM output, which will make this library treeshakeable, saving some more KBs for consumers. I suggest trying TSDX for this.

PS, Babel CLI docs aren't super updated, so better run yarn babel -h to see the latest options.

@adi518 adi518 force-pushed the remove-core-js branch 3 times, most recently from 22a0bdb to eb47af2 Compare October 20, 2020 22:23
@@ -50,7 +50,7 @@
"sideEffects": false,
"main": "build/index.js",
"scripts": {
"build": "babel --out-dir build src --ignore src/__tests__",
"build": "babel --out-dir build src --ignore src/__tests__ --source-maps --delete-dir-on-start --minified --no-comments",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we also need a NODE_ENV=production ?

Copy link
Contributor Author

@adi518 adi518 Oct 21, 2020

Choose a reason for hiding this comment

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

I don't think so, but I'll check.

@tizmagik
Copy link
Collaborator

Thanks, this is great! Do you mind also adding a note in the docs about the needed polyfills for older browser support moving forward after this change?

And yes, to answer your question, I think to be safe we will consider this a major breaking change.

@tizmagik
Copy link
Collaborator

Use Webpack 5 or Rollup to build an ESM output, which will make this library treeshakeable, saving some more KBs for consumers. I suggest trying TSDX for this.

Great idea. Happy to accept PRs for these as well (or at least an issue).

@adi518
Copy link
Contributor Author

adi518 commented Oct 21, 2020

Sure, I'll update the readme.

@adi518 adi518 force-pushed the remove-core-js branch 2 times, most recently from f45d606 to 7beec4c Compare October 21, 2020 17:27
@adi518 adi518 requested a review from tizmagik October 21, 2020 17:28
Copy link
Collaborator

@tizmagik tizmagik left a comment

Choose a reason for hiding this comment

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

Thanks! I might try and get a minor release out under v8 before merging this for a v9 release but approving and will hold for now 👍

@patrickskim
Copy link

core-js was an issue we've been having as well! Please merge & release ASAP!

@tizmagik
Copy link
Collaborator

tizmagik commented Nov 2, 2020

Yes, looking to do a release this week hopefully. I want to land #168 as semver minor in v8 first

@tizmagik
Copy link
Collaborator

tizmagik commented Nov 3, 2020

@patrickskim @adi518 looks like #168 might not land this week so I've published this PR against @alpha premajor release:

npm install react-tracking@9.0.0-0

Try it out and please post back with any issues you come across. Thanks!

@tizmagik
Copy link
Collaborator

@patrickskim @adi518 have you encountered any issues with this premajor?

@adi518
Copy link
Contributor Author

adi518 commented Nov 10, 2020

I haven't pushed to production with the new version yet, but it's ongoing.

@antciccone
Copy link

@tizmagik We were able to upgrade to react-tracking@9.0.0-0 in our gatsby project. It has been pretty seamless so far. Will update you if @patrickskim or I run into any issues! Thanks for this work 🔥

@tizmagik
Copy link
Collaborator

v8.1.0 is released. I'll look to release a v9.0.0 next week or so with this change. Thanks again!

@adi518
Copy link
Contributor Author

adi518 commented Dec 6, 2020

We are about two weeks in production with 9.0.0-0 and thus far, no issues.

@tizmagik
Copy link
Collaborator

tizmagik commented Jul 14, 2021

RC v9.0.0-4 has been tagged, mind giving that a quick check and if all looks good I'll merge, tag and release this to mainline @adi518 @antciccone @patrickskim

npm install react-tracking@9.0.0-4

@gedeagas
Copy link
Contributor

Removing core js make the bundle size much smaller. 15.3kb and 2.42 when gzip.
Screen Shot 2021-07-15 at 12 41 08 AM

So far no issues, i will run the build on staging for a few days and wait the result.

Thank you for pushing this @tizmagik 🎉

@gedeagas
Copy link
Contributor

@tizmagik after few days of dogfooding and thus far, no issues. I think this release is good to go.

🚢It 🎉

@tizmagik
Copy link
Collaborator

Great, thanks @gedeagas !

@tizmagik
Copy link
Collaborator

Hey @bgergen do you think we can get CI going again? I'd love to have it test this PR before we merge/publish, if possible

@bgergen
Copy link
Collaborator

bgergen commented Jul 19, 2021

@tizmagik Yes! I'll try to get that setup tomorrow. Sorry, I've been swamped today getting ready for Olympics coverage.

@bgergen
Copy link
Collaborator

bgergen commented Jul 21, 2021

@tizmagik Working on figuring out why this isn't triggering a drone build. Maybe I missed something in the config to get it to run on a PR from a fork.

@tizmagik
Copy link
Collaborator

Thanks @bgergen -- feel free to push directly to this branch

@tizmagik
Copy link
Collaborator

Yay thank you everyone for all the hard work! 🎉

@tizmagik tizmagik merged commit b52726b into nytimes:master Jul 22, 2021
@tizmagik
Copy link
Collaborator

This is now released as v9.0.0 thanks again all!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants