feat: accept Uint8Arrays input in place of Buffers

[Gozala]

This pull request relaxes input type from Buffer to Uint8Array. It does however depends on multiformats/js-multibase#61

[vmx]

I've the same questions as @rvagg (thanks for bringing it up) multiformats/js-multibase#61 (comment):

Does the require('util') mean we need to include it in the dependencies list to make sure we have it available for webpack@5?

Could you please add a test that is using a Buffer as input, to make sure it still works?

Am I correct that this change depends on multiformats/js-multibase#61?

[Gozala]

I've the same questions as @rvagg (thanks for bringing it up) multiformats/js-multibase#61 (comment):

I'm not sure what changes with webpack@5 but:

1. It is only in tests
2. It is only relevant for <=node@10 I think. So if we don't care testing in older versions of node, I can remove it.

Could you please add a test that is using a Buffer as input, to make sure it still works?

Ok, I'll do that

Am I correct that this change depends on multiformats/js-multibase#61?

Yep, as description said 🤓

[Gozala]

• Have test run with both Buffer and Uint8Array inputs.

  Could you please add a test that is using a Buffer as input, to make sure it still works?

• Imported web-encodings library to deal with that

  Does the require('util') mean we need to include it in the dependencies list to make sure we have it available for webpack@5?

[mikeal]

If it’s only in tests it’s probably fine, the primary concern is that bundlers are not injecting polyfills for Node.js anymore.

At some point we may even want it out of the tests, and we should all be getting in the habit of working without Node.js stdlib as much as possible, but it’s not a blocker.

We’ve cut Node.js v10 elsewhere but I’m not 100% sure what our dependencies have done that rely on this library.

[Gozala]

If it’s only in tests it’s probably fine, the primary concern is that bundlers are not injecting polyfills for Node.js anymore.

At some point we may even want it out of the tests, and we should all be getting in the habit of working without Node.js stdlib as much as possible, but it’s not a blocker.

We’ve cut Node.js v10 elsewhere but I’m not 100% sure what our dependencies have done that rely on this library.

web-encoding should take care of providing TextEncoder / TextDecoder across the board. And it's tested on node v10 as well as 12 and 14

[vmx]

Overall looks good, but I think there is a superfluous file, see inline.

[vmx]

See previous inline comment.

[vmx]

Approved, though I'd like CI to run and be green before it is merged.

[Gozala]

Approved, though I'd like CI to run and be green before it is merged.

Once #89 lands, I can run CI tests.

[achingbrain]

Once #89 lands, I can run CI tests.

This has landed now and the tests are all green, but for the future you could have used that branch as the merge base for this PR which would have let you run CI before it was merged.