-
Notifications
You must be signed in to change notification settings - Fork 23
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
feat: support Uint8Array input in place of Buffer #13
Conversation
Thank you for submitting this PR!
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.
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. |
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. |
@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 Clicking around doesn't provide much info either & I'm really not sure how any of these changes affect coverage. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Could you please open a draft PR against js-ipfs with this change in the dep tree so we can see that it's not going to inadvertently break anything? You may need to push branches to a few repos to make sure all the versions of protons align.
@achngbrain this is what this pull attempt to do ipfs/js-ipfs#3070 |
@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): 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 |
With this change all the
Buffer
inputs were replaced byUint8Array
which is backwards compatible sinceBuffer
is a subclass.This does make an assumption that "bytes" encoder used to take
Buffer|string
here:protons/src/compile/encodings.js
Lines 30 to 43 in 3b5276f
Fixes #12