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

Switch from babel to buble #6836

Closed
wants to merge 2 commits into from
Closed

Switch from babel to buble #6836

wants to merge 2 commits into from

Conversation

benmccann
Copy link
Contributor

@benmccann benmccann commented Dec 15, 2019

As suggested by @leeoniya in #6835. Thanks @leeoniya!

Builds much faster and results in a slightly smaller minifed file

$ gulp build
[15:16:38] Using gulpfile ~/src/Chart.js/gulpfile.js
[15:16:38] Starting 'build'...

src/index.js → dist/Chart.esm.js...
created dist/Chart.esm.js in 5s

src/index.js → dist/Chart.esm.min.js...
created dist/Chart.esm.min.js in 6.2s

src/index.js → dist/Chart.js...
created dist/Chart.js in 3.5s

src/index.js → dist/Chart.min.js...
created dist/Chart.min.js in 6.4s
[15:16:59] Finished 'build' after 22 s

$ ls -lh dist
total 1.3M
811 Dec 15 15:10 Chart.css
811 Dec 15 15:10 Chart.esm.css
462K Dec 15 15:10 Chart.esm.js
521 Dec 15 15:10 Chart.esm.min.css
176K Dec 15 15:10 Chart.esm.min.js
463K Dec 15 15:10 Chart.js
521 Dec 15 15:10 Chart.min.css
177K Dec 15 15:10 Chart.min.js

$ gulp build
[15:14:10] Using gulpfile ~/src/Chart.js/gulpfile.js
[15:14:10] Starting 'build'...

src/index.js → dist/Chart.esm.js...
created dist/Chart.esm.js in 2s

src/index.js → dist/Chart.esm.min.js...
created dist/Chart.esm.min.js in 4.3s

src/index.js → dist/Chart.js...
created dist/Chart.js in 1.5s

src/index.js → dist/Chart.min.js...
created dist/Chart.min.js in 4.3s
[15:14:22] Finished 'build' after 13 s

$ ls -lh dist
total 1.2M
811 Dec 15 15:11 Chart.css
811 Dec 15 15:11 Chart.esm.css
425K Dec 15 15:11 Chart.esm.js
521 Dec 15 15:11 Chart.esm.min.css
174K Dec 15 15:11 Chart.esm.min.js
426K Dec 15 15:11 Chart.js
521 Dec 15 15:11 Chart.min.css
174K Dec 15 15:11 Chart.min.js

Copy link
Member

@etimberg etimberg left a comment

Choose a reason for hiding this comment

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

I'm ok with this if we also drop IE11 support and only target the latest browsers.

rollup.config.js Show resolved Hide resolved
@benmccann
Copy link
Contributor Author

I'm ok with this if we also drop IE11 support and only target the latest browsers.

Why would it be necessary to drop IE support to make this change? buble transpiles to ES5 which is supported on IE11, so IE11 should work just fine with this change

@benmccann
Copy link
Contributor Author

Hmm. It seems that gulp test --coverage is broken. It works without the --coverage flag which is why I didn't notice any problems in my own testing

@benmccann
Copy link
Contributor Author

benmccann commented Dec 18, 2019

I had tried getting it to work by replacing istanbul with c8, but I'm not quite sure it'll be an option in the short-term at least: bcoe/c8#175

@kurkle
Copy link
Member

kurkle commented Dec 19, 2019

Did you test with IE if the build works ok?

@benmccann
Copy link
Contributor Author

No, I don't have a windows machine, so don't have a way to test. I'd love it if you have IE available if you're able to check

@kurkle
Copy link
Member

kurkle commented Dec 25, 2019

IE: Object doesn't support property or method 'assign'
(and on master: Object doesn't support property or method 'entries')

@kurkle
Copy link
Member

kurkle commented Dec 25, 2019

So, dug into this a bit. Buble uses Object.assign instead of spread operator.
Can't use Chart.helpers.extend, because helpers is build using spread operator.
Could add a polyfill for Object.assign, or change the layout so that extend can be used. But IMO this would be waste of time for supporting a soon to be completely dead product.

Those Object.entries will need to be rewritten to use Object.keys instead, for IE support.

StatCounter / IE:
WorldWide: 1.66% (went down 1,15% between 11/18-11/19)
USA: 2.73% (went down 2,33% between 11/18-11/19)

@benmccann
Copy link
Contributor Author

Thanks so much for testing this out! There's not really any information on how to use polyfills with buble that I can easily find. Perhaps it's better just to stick with babel since it's more widely used and better documented and things are working a bit better with it

@benmccann benmccann closed this Dec 26, 2019
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

3 participants