Skip to content
This repository has been archived by the owner on Aug 24, 2021. It is now read-only.

feat: accept Uint8Arrays in place of Buffers #61

Merged
merged 4 commits into from
Jul 24, 2020

Conversation

Gozala
Copy link
Contributor

@Gozala Gozala commented Jul 22, 2020

This pull request relaxes input type from Buffer to Uint8Array. It still returns Buffers back to ensure backwards compatibility. It also adds missing TS JSDoc comments as they helped me understand interfaces between components.

@rvagg
Copy link
Member

rvagg commented Jul 22, 2020

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

@Gozala
Copy link
Contributor Author

Gozala commented Jul 22, 2020

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

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

  1. It is only for testing on <=node@10 otherwise TextEncoder/TextDecoder are available as globals.
  2. I am only realizing now this doesn't even run on CI so I could probably just assume we're on at least LTS
  3. If we need to add dep for webpack@5 it's probably better to just switch to global or have a separate module that takes just exposes TextEncoder / TextDecoder

@Gozala
Copy link
Contributor Author

Gozala commented Jul 22, 2020

Oh I was wrong in suggesting that:

  • It is only for testing on <=node@10 otherwise TextEncoder/TextDecoder are available as globals.
  • I am only realizing now this doesn't even run on CI so I could probably just assume we're on at least LTS

I do see now that this is not the case, it is used in the code not just test

@Gozala
Copy link
Contributor Author

Gozala commented Jul 22, 2020

@rvagg I have addressed your remark by pulling in web-encoding dependency that just provides TextEncoder / TextDecoder across various configurations.

@rvagg
Copy link
Member

rvagg commented Jul 23, 2020

You need to cut a new version of web-encoding @Gozala, the 1.0.1 package is 110Mb and unpacked is 356Mb thanks to a bundled version of Linux Chromium in a .cache directory.

btw if this comes from consuming puppeteer then check out https://github.com/rvagg/polendina#minimising-puppeteers-size to reduce the size, having ~300Mb extra junk in each of my package directories even for devdeps drives me mad.

Copy link
Member

@rvagg rvagg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, although I think it should get a ^1.0.2 for web-encoding before releasing though, and you might want to consider tweaking the README to mention Uint8Arrays, it's a bit ambiguous at the moment with "buffer" (lower-case) and that could become "Buffer or Uint8Array".

@Gozala
Copy link
Contributor Author

Gozala commented Jul 23, 2020

You need to cut a new version of web-encoding @Gozala, the 1.0.1 package is 110Mb and unpacked is 356Mb thanks to a bundled version of Linux Chromium in a .cache directory.

btw if this comes from consuming puppeteer then check out https://github.com/rvagg/polendina#minimising-puppeteers-size to reduce the size, having ~300Mb extra junk in each of my package directories even for devdeps drives me mad.

I'm guessing it is coming from playwright-test, which I end up using because I could not figure out rvagg/polendina#10. BTW you and @hugomrdias might want to consider converging two.

@Gozala
Copy link
Contributor Author

Gozala commented Jul 23, 2020

You need to cut a new version of web-encoding @Gozala, the 1.0.1 package is 110Mb and unpacked is 356Mb thanks to a bundled version of Linux Chromium in a .cache directory.

I end up adding .cache to .npmignore to solve this.

@hugomrdias
Copy link
Member

There so not be any .cache or bundle browser in the playwright-test npm package install, it lazy downloads the browser binary to the OS temp folder only once per version, if this is not happening please open an issue and i will fix it!

@hugomrdias hugomrdias changed the title Accept Uint8Arrays in place of Buffers feat: accept Uint8Arrays in place of Buffers Jul 24, 2020
@hugomrdias hugomrdias merged commit abc1595 into multiformats:master Jul 24, 2020
@hugomrdias
Copy link
Member

Thank you @Gozala

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants