hildjj / node-cbor

Encode and decode CBOR documents, with both easy mode, streaming mode, and SAX-style evented mode.
MIT License
362 stars 74 forks source link

Publish library as ES5 #87

Closed Hexxeh closed 5 years ago

Hexxeh commented 6 years ago

This change addresses https://github.com/hildjj/node-cbor/issues/60 by running Babel over the code before publishing to npm, transpiling the code down to ES5.

The tests still pass and execute against the original code. Not sure what else I need to check to make sure I haven't broken it, but if you have concerns let me know!

coveralls commented 6 years ago

Coverage Status

Coverage remained the same at 99.713% when pulling 380315e728127ba06f68085886cbebd5296a766c on Hexxeh:feature/es5-publish into 708eac884e0829424c262904e5e97764c634029d on hildjj:master.

coveralls commented 6 years ago

Coverage Status

Coverage decreased (-1.0%) to 98.756% when pulling 8dd44a53a006c5420cdf2c90c37ffee4e340c6c4 on Hexxeh:feature/es5-publish into 708eac884e0829424c262904e5e97764c634029d on hildjj:master.

hildjj commented 6 years ago

The need to move back to src and lib makes me sad. In particular, I'd like node users to get the un-babel'd version.

Hexxeh commented 6 years ago

We could set the "browser" field in package.json to point to the transpiled copy, and set "main" to the original version in src? I can make that change now if you'd accept the change with that.

hildjj commented 6 years ago

i think that helps. could you also move the originals back to lib, and do the browser build into a browser directory (or whatever you want to name it)?

hildjj commented 6 years ago

Please also add yourself to the contributors list in package.json.

Hexxeh commented 6 years ago

Done, but the coveralls check is failing, but there's no source changes at this point, so that seems like it might be a false-positive.

hildjj commented 6 years ago

nod. don't worry about the coveralls change. I'll pull this down and play with it over the weekend, which may need another tweak or two. Thanks for the PR!

Hexxeh commented 6 years ago

Sounds great, please let me know if any further changes are needed. I'm hoping to avoid temporarily forking this package to pull it into our build, so I'm happy to do anything I can to get this into a published release ASAP. 😄

hildjj commented 6 years ago

i'm sorry to keep asking for more things. how did you test this? can we make it easy for other people to test it in a browser?

Firefox is my highest priority.

hildjj commented 6 years ago

I bet @linuxwolf could help us figure that out.

Hexxeh commented 6 years ago

Could use Karma but it wouldn't tell us much in the case of this PR when run against modern browsers since those support >ES5 already.

I've been testing by yarn linking this into our create-react-app project and checking that the production build succeeds.

hildjj commented 6 years ago

Is your create-react-app project public? Matt doesn't have time to help us this week, so let's talk requirements:

Hexxeh commented 6 years ago

Unfortunately not, but any create-react-app that does:

import cbor from 'cbor;
cbor.encode({a: 1});

and then tries to run a production build will fail to build without this change.

I don't believe it's possible to exclude the browser files from ending up on disk for Node users, in the same way that browser developers will get the Node version on disk too.

Ava can't run tests in a browser: https://github.com/avajs/ava/blob/master/docs/recipes/browser-testing.md (it can try to emulate a browser like environment, but nothing more). Karma is designed for this.

Is having a browser test environment a blocker for landing a change to producing an ES5 build? It's not browser specific, it's just a lowest-common-denominator that build tools and engines accept.

hildjj commented 6 years ago

Having a browser test environment is not a strict blocker, but I'd like to see it work once. In particular, I don't understand how this relatively-innocuous translation actually helps matters that much. It appears as if this just makes us ES5-safe, but doesn't do the browserify steps to deal with polyfilling Buffer, etc.

Hexxeh commented 6 years ago

create-react-app uses a version of uglifyjs that can't parse ES6. You can use ES6 features in your own code, and that'll work because it gets transpiled with Babel, but it doesn't transpile dependencies. It still provides polyfills for Buffer because everything still runs through Webpack.

In general I guess transpiling dependencies is avoided because it's slower (and less fraught with edge-cases) than transpiling at publish-time?

Hexxeh commented 6 years ago

Anything I can do to help get this landed?