ipfs / protons

Protocol Buffers for Node.js and the browser without eval
Other
32 stars 23 forks source link

feat: support Uint8Array input in place of Buffer #13

Closed Gozala closed 4 years ago

Gozala commented 4 years ago

With this change all the Buffer inputs were replaced by Uint8Array which is backwards compatible since Buffer is a subclass.

This does make an assumption that "bytes" encoder used to take Buffer|string here:

https://github.com/ipfs/protons/blob/3b5276f052f2e17c2d806d27cd9a88e156588977/src/compile/encodings.js#L30-L43

Fixes #12

welcome[bot] commented 4 years ago

Thank you for submitting this PR! A maintainer will be here shortly to review it. We are super grateful, but we are also overloaded! Help us by making sure that:

Getting other community members to do a review would be great help too on complex PRs (you can ask in the chats/forums). If you are unsure about something, just leave us a comment. Next steps:

We currently aim to provide initial feedback/triaging within two business days. Please keep an eye on any labelling actions, as these will indicate priorities and status of your contribution. We are very grateful for your contribution!

Gozala commented 4 years ago

I have propagated changes from the ipfs-utils and addressed comments from @hugomrdias

I do not understand what is the problem here https://codecov.io/gh/ipfs/protons/compare/3b5276f052f2e17c2d806d27cd9a88e156588977...8ee9c9febd7c8e1e0d348a53a15c03721d81ce25 navigating to that URL does not really provide any info. Any help on resolving that would be greatly appreciated.

Gozala commented 4 years ago

@rvagg I have added node 14 as requested. Tried adding 'current' as well but Travis seems unable to resolve that on on windows and failing.

@achingbrain @hugomrdias is there anything else I need to do to get this merged ?

I also have no clue what to do regarding codecov failure there. Going there this is what I see

image

Clicking around doesn't provide much info either & I'm really not sure how any of these changes affect coverage.

Gozala commented 4 years ago

@achngbrain this is what this pull attempt to do https://github.com/ipfs/js-ipfs/pull/3070

Gozala commented 4 years ago

@achingbrain you've asked me yesterday to update libp2p with new protons to ensure things don't break. I've started looking into it and found out that there quite few other dependencies more levels deep:

https://www.npmjs.com/package/protons

However we also discovered that even though I am not updating libp2p and those other dependencies bundle ends up with my patched version (I'm guessing because version is the same):

image

That is to suggest that all IPFS tests run with protons changes.

So is there anything else needs to be done to land this ? If so could you please let me know ?

Thanks