Skip to content
This repository has been archived by the owner on Jun 17, 2021. It is now read-only.

Added babel-cli to distribute ES5 version on npm #114

Merged
merged 3 commits into from Feb 1, 2018
Merged

Added babel-cli to distribute ES5 version on npm #114

merged 3 commits into from Feb 1, 2018

Conversation

holgerd77
Copy link
Member

Adresses #111 and #92

@coveralls
Copy link

coveralls commented Jan 29, 2018

Coverage Status

Coverage increased (+0.003%) to 99.17% when pulling 121e850 on es5-dist into 757ba73 on master.

@nickysemenza
Copy link

I'd love to see this merged 😄

@holgerd77 holgerd77 requested a review from Silur February 1, 2018 09:50
@holgerd77 holgerd77 changed the title [WIP] Added babel-cli to distribute ES5 version on npm Added babel-cli to distribute ES5 version on npm Feb 1, 2018
@holgerd77
Copy link
Member Author

@nickysemenza This is now in review, hope to have this merged soon.

@@ -1,5 +1,5 @@
var assert = require('assert')
var ethUtil = require('../index.js')
var ethUtil = require('../dist/index.js')
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe tests can be part of the babel space too for some may write these in ES6?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, yes, we can think about that, but let's leave this for now.

@Silur Silur merged commit 3550ee3 into master Feb 1, 2018
{
"presets": [
[
"env"
Copy link
Contributor

Choose a reason for hiding this comment

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

If publisher will build with latest node this will produce not ES5 output, is not it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Could you be a bit more specific, who is the "publisher", the one who publishes this on npm? And what "latest" node do you mean, which version? What changed in that version, do you have some link on that?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, one missed word destroyed my message, I wanted write: If publisher will build with latest node version...

I checked https://www.npmjs.com/package/babel-preset-env

Without any configuration options, babel-preset-env behaves exactly the same as babel-preset-latest (or babel-preset-es2015, babel-preset-es2016, and babel-preset-es2017 together).

Did not knew about this. Sorry for false alarm.

Copy link
Member Author

Choose a reason for hiding this comment

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

No problem, I am not totally the transpilation/babel expert, so better to double check than one check to few. Sorry also for being a bit unconsistent in what Node version we actually support. I think we still have to fine-tune this a bit. I also didn't know how many people still need some ES5 compatible distribution and for what reasons.

So I think it probably makes sense for the various libraries to -yes- drop Node 4 support for the source version to be able to implement with newer and more elegant techniques, but also provide a transpiled distribution version with lower requirements.

Probably also has to be decided/evolved on a case-by-case (library-by-library) basis, depending on the needs of the users, the scope of the library and the compromises which have to be done in respect to security, speed and build/library size.

Actually this topic gets more and more complex for me the more I read on it. Will try to keep pace! 😄

@fanatid fanatid deleted the es5-dist branch February 2, 2018 09:06
@gre
Copy link

gre commented Feb 3, 2018

Is there any chance this will get backport to the 4.x release? many library still depend on the 4.x , like ethereumjs-abi and it will take time for everyone to update to it.
Thanks

@holgerd77
Copy link
Member Author

@gre Hmm, not very probable, sorry, there is so much to do on the various libs, don't think anyone will find the time. Is ethereumjs-abi your main concern? I would rather look into updating this instead then.

@gre
Copy link

gre commented Feb 4, 2018

ok thanks @holgerd77 . I've sent a PR for -abi ethereumjs/ethereumjs-abi#59 , i'm going to see which other dependency I have still uses the 4.x.
maybe later, you guys should consider doing a monorepo & using things like lerna. save lot of times ( see for instance https://github.com/0xProject/0x.js or https://github.com/LedgerHQ/ledgerjs )
cheers

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

Successfully merging this pull request may close these issues.

None yet

7 participants