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

fix: dont rely in node globals #81

Closed

Conversation

hugomrdias
Copy link
Contributor

This PR removes the dependency on node globals, so the users don't need to rely on browser bundlers magic.

related to ipfs/js-ipfs#2924

This PR removes the dependency on node globals, so the users don't need to rely on browser bundlers magic.

related to ipfs/js-ipfs#2924
Copy link
Collaborator

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@rvagg
Copy link
Owner

rvagg commented Mar 17, 2020

Here's the counterpoint to doing this:

Current package weight on disk, 334K:

bl@4.0.1
└─┬ readable-stream@3.6.0
  ├── inherits@2.0.4
  ├─┬ string_decoder@1.3.0
  │ └── safe-buffer@5.2.0
  └── util-deprecate@1.0.2

New weight on disk, 476K:

bl@4.0.1
├─┬ buffer@5.5.0
│ ├── base64-js@1.3.1
│ └── ieee754@1.1.13
├── inherits@2.0.4
└─┬ readable-stream@3.6.0
  ├── inherits@2.0.4 deduped
  ├─┬ string_decoder@1.3.0
  │ └── safe-buffer@5.2.0
  └── util-deprecate@1.0.2

They're not large dependencies but given how many instances of bl there are in the wild, are we OK with bloating disk like this?

@hugomrdias what's the user experience looking like with bundlers in this brave new world they're ushering in? Is it lashings of JSON syntax to manually wire up each of these items, or just a "node": true? And can you expand on the js-ipfs concern that's pushing this? Is it just a desire to not be that one dependency that needs special attention?

@hugomrdias
Copy link
Contributor Author

Rollup never supported node polyfills by default, webpack 5 is going to remove them. Even with node plugins in rollup and webpack 4 the polyfills injected are super outdated (i tried to upgrade some in webpack no response there's PRs with 4 years), which causes duplication like crazy and interop issues.

counter point is that at least buffer and inherits are wildly used so bloating should minimal plus we can improve readable-stream with TextEncoder and reduce the size.

@rvagg
Copy link
Owner

rvagg commented Mar 18, 2020

v4.0.2

@rvagg rvagg closed this Mar 18, 2020
@hugomrdias hugomrdias deleted the fix/dont-rely-in-node-globals branch March 18, 2020 10:34
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.

3 participants