Convert codebase to ES6 and reduce dependency on Node-specific modules #422
Conversation
There was a problem hiding this comment.
This is a huge change - thanks for taking the time @valadaptive!
No major concerns, please take a look at the inline comments. Did you use a tool to do the translation? I'm curious how difficult it would be to swap the lets for consts where possible.
The translation to ES6 class syntax was done manually. The translation from There are tools like lebab that can automatically convert to ES6 classes as well, but it seems much less well-maintained than ESLint, so I opted to go with ESLint + the manual approach. It should be fairly easy to use |
ESLint is what most people use nowadays, but the repository is currently set up for JSHint, so go with that for now.
ESLint is much more flexible in its configuration, letting us replace the jshint comments at the top of each file with a single configuration file. We can also enforce more code style. This only fixes things like whitespace and quotes, leaving other lint fixes for the next commit.
These lints consist of unsafe uses of hasOwnProperty and unused variables.
ES6 lets use use let and const statements, which are block-scoped, making them much nicer to work with. Removing the rest of the var statements and cleaning up will be done in the next commits.
In preparation for making Tap a class
This includes a breaking change: extending types requires actually subclassing them. It would be nice to throw an error when that's not done, but nothing like that is implemented right now.
Defining them on the prototype caused a performance regression in isValid due to JS engine voodoo.
Uint8Array.prototype.slice.call is faster than allocating a buffer then copying the existing buffer into it. This could be because it's implemented in native code, because it doesn't need to zero out the buffer first, or similar reasons. This gives up to a 30% performance boost on primitive-heavy types.
This is a breaking change. There isn't a good drop-in replacement for zlib in the browser; browserify-zlib was last updated 6 years ago and more up-to-date zlib libraries like pako have incompatible API surfaces. It should be very easy for avsc users to re-add deflate as a codec if they need it--there's nothing for them to reimplement, and they just need to pass zlib.[in/de]flateRaw as a codec (as is done in the tests).
This allows us to stub path-related functionality in the browser.
Now that the minimum Node version is 6, these will always be available
Because let is block-scoped, we can get rid of a lot of declarations.
This is especially handy for module.exports
This reverts commit c9e05d7.
|
Thank you very much for this @valadaptive! Follow up release PR: #428 |
This PR does a few things:
avro.Typeclasses (especiallyLogicalType) must use ES6extends.utilhas been almost entirely removed.deprecateanddebuglogare the only remaining use cases, and they're put behind shims that are no-ops in the browser.pathhas been abstracted behind the existingfiles.jscompatibility layer.zlibhas been removed by removing'deflate'as a default codec, which is the second breaking change. Fortunately, it's very easy to add back for any API consumers--they can just pass inzlib.[in/de]flateRawas the codec.bufferandstream, which have robust polyfills currently available on NPM. Removing these two dependencies,bufferespecially, is a more complex undertaking, but I feel better about importing them since they're a core part of the API surface when compared to, say,zlib, which was essentially only imported on behalf of the API consumer.letstatements are lexically scoped, and deprecatednew Buffercalls have been completely removed now that Node < 6 no longer needs to be supported.toBufferwhich results in a ~30% performance improvement. Overall, this PR seems to be within the regular margin of error for performance, with some tests slightly improving and others slightly worsening, but I found an optimization just to be on the safe side.