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

refactored lib/types and lib/utils to make rollup friendly #224

Closed
wants to merge 4 commits into from

Conversation

hath995
Copy link

@hath995 hath995 commented Feb 25, 2019

This is more of a request for comment than a real pull request. I would like to use avsc in the browser using the packager Rollup. Currently, I could not get that to work to with the standard distribution. I did try browserify and that worked of course, but it doesn't really help me with my bundle.

I'm also trying to minimize the weight of the library. With the included build.js and rollup I could push the size down to 68k after this refactor (which is mostly neutral but may have broken one or two things)

So in this pr I removed as much node.js code as I could from lib/util and lib.types. To do that I refactored the types to the es6 class syntax to remove the dependency on util.inherits. As removing the node.js formatting and debug functions. There are two places which I added @fixme notices where that probably broke something. I haven't run any of the tests yet.

Overall, it would be helpful if there was an esm distribution of avsc, which would probably be a different way to make this library rollup friendly.

So what would you recommend on how to proceed?

@mtth
Copy link
Owner

mtth commented Mar 2, 2019

Hi @hath995, TIL about rollup; thanks for the PR.

Is it possible to split the "weight reduction" part of this PR into a separate PR? Supporting rollup sounds like a net win but I am hesitant to also introduce an ES6 requirement in avsc.

If you only care about the de/serialization part of this library, another option would be to merge this into @avro/types instead of avsc. Currently it is identical to the types portion of avsc (see the packages branch in this repository) but going forward I am planning on adding a few features, probably relying on ES6.

@hath995
Copy link
Author

hath995 commented Mar 3, 2019

I can do a more minimal pr.

I think moving to es6 is generally a good idea. Given how principled this library is about types it should also be pretty easy to convert it to Typescript as well. Then folks could compile it to whatever target language version they wanted. Babel could do the same though.

One issue I can describe in more detail now. Here
https://github.com/hath995/avsc/blob/rollup/lib/types.js#L2694
You initialize a value on this before you call the parent class constructor which breaks in es6 which disallows assigning values to this before calling the parent constructor. As a way of future proofing the library it would be good to find an alternative there.

@hath995 hath995 mentioned this pull request Mar 12, 2019
@hath995 hath995 closed this Mar 12, 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.

2 participants