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: add buffer #632

Merged
merged 1 commit into from
May 3, 2020
Merged

fix: add buffer #632

merged 1 commit into from
May 3, 2020

Conversation

hugomrdias
Copy link
Contributor

This PR adds buffer as a dependency to avoid relying on bundlers automatic node globals injection.

@t-mullen t-mullen merged commit b802547 into feross:master May 3, 2020
@feross
Copy link
Owner

feross commented May 10, 2020

This PR should not have been merged IMO. There are several issues:

  1. require('buffer') always uses the Node.js buffer, ignoring the buffer package from npm which is installed in node_modules. To use the npm version, you need to do require('buffer/').

  2. We don't actually want to use the npm version when running in Node.js since it's less efficient and compatible than the built-in Buffer.

@feross feross mentioned this pull request May 10, 2020
@hugomrdias
Copy link
Contributor Author

hugomrdias commented May 11, 2020

  1. Exactly but if you use bundlers without node builtins injection you get the npm package
  2. This package will use node buffer when in node, exactly why I didn't use require('buffer/')

So this works exactly the same way as before plus supports bundlers without injection.

@hugomrdias
Copy link
Contributor Author

hugomrdias commented May 12, 2020

@feross any chance i can convince you to reimplement this ?
This PR is part of a larger endeavour to remove node globals and built ins from ipfs so we don't rely on bundlers to inject node stuff and prepare for webpack 5, we already worked with the maintainers from bl, readable-stream, leveldb, etc to make this a reality would be really important to have simple-peer too.

@EricEisaman
Copy link

I agree. As we prepare for Deno to take the stage, removing anything Node specific is a wise choice.

@feross
Copy link
Owner

feross commented May 12, 2020

@hugomrdias Can you link the PRs from those other projects so we can take a look?

@hugomrdias
Copy link
Contributor Author

Level/level-js#191
Level/abstract-leveldown#362
rvagg/bl#81
nodejs/readable-stream#435 (this one is still wip but not because of buffer)

@nazar-pc
Copy link
Collaborator

Can't we have ./buffer.js that will re-export native Buffer in Node.js and polyfill in browsers using browser field in package.json?

@hugomrdias
Copy link
Contributor Author

The changes in the PR basically do that, but the node/browser dance is inside the buffer package.

@vasco-santos
Copy link

Hey folks! Is there any update on this?

@t-mullen
Copy link
Collaborator

t-mullen commented Jun 30, 2020

@feross I disagree with the revert of this PR.

require('buffer') always uses the Node.js buffer, ignoring the buffer package from npm which is installed in node_modules. To use the npm version, you need to do require('buffer/').

We never used the NPM module before in Node.js and this PR does not change that. The intent here is not to start using the NPM version either. There's no change in Node.js here.

We don't actually want to use the npm version when running in Node.js since it's less efficient and compatible than the built-in Buffer.

I agree, and this PR doesn't stop using native buffer or intend that.

The only change here is with the browser. Currently browserify will see require('buffer') and automatically inject the NPM version. Not all bundlers do this and will fail due to the non-existent buffer dependency. Adding the dependency explicitly will allow these bundlers to work.

@feross
Copy link
Owner

feross commented Nov 11, 2020

@t-mullen Yep, you're right. I just reverted my revert and shipped this in 9.9.0.

FredZeng pushed a commit to FredZeng/simple-peer that referenced this pull request Oct 14, 2023
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.

6 participants