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

Remove is-buffer dependency #2375

Closed
wants to merge 2 commits into from
Closed

Conversation

mvanaltvorst
Copy link

feross has been thinking about including ads in one of his other libraries. I think we should take preventive measures by trying to rely less on his packages.

@mvanaltvorst
Copy link
Author

Tests seem to be failing because of an unrelated issue, don't mind them.

@DominikSerafin
Copy link

DominikSerafin commented Aug 26, 2019

Duplicate of #1816

Otherwise I'm greatly in favor of inlining is-buffer.

I'm wondering what was actually thought process here when including this dependency 🤔. Since axios got only 2 depencies: is-buffer and follow-redirects.

RyanLafferty added a commit to RyanLafferty/axios-is-buffer-removed that referenced this pull request Aug 26, 2019
@feross
Copy link

feross commented Aug 28, 2019

feross has been thinking about including ads in one of his other libraries.

I ran funding as an experiment to start a conversation about open source sustainability. The experiment is now over. The maintainers are free to remove is-buffer if they'd like. But don't do it because brigaders and trolls from this hate-filled r/programming thread tell you to do it. ;)

@DominikSerafin
Copy link

@feross personally I'd like to see is-buffer inlined not because of r/programming or the funding experiment, but because it's such a tiny snippet of code I don't think it belongs as a dependency. And from my experience - the less dependencies, the better the project is in terms of:

  • Maintenance
  • Predictability & Stability
  • Security

I'd love to get to know your perspective though - why do you think it's beneficial to have it included as dependency?

@feross
Copy link

feross commented Sep 5, 2019

In my experience, I don't inline anything but the most trivial code because even code that initially seems trivial may need to be updated due to platform changes or previously undiscovered edge case bugs. In the past, is-buffer had many such changes. I wrote a bit about some of the changes it has been through here: https://twitter.com/feross/status/1166531085410263040

Another one I didn't mention in that post is that Node used to have a SlowBuffer class that required special handling in is-buffer. I removed the special handling after we dropped Node 0.10 support.

At this point, I think that is-buffer has probably settled down enough that inlining it is reasonably safe. But even though it's super unlikely, there's no absolute guarantee that buffer won't change in some way that requires is-buffer to be updated.

Ultimately, it's your call 🌟

@yasuf
Copy link
Collaborator

yasuf commented Nov 18, 2019

Closing, just merged #1816, thanks for helping out even though this PR didn't get merged!

@yasuf yasuf closed this Nov 18, 2019
@axios axios locked and limited conversation to collaborators May 3, 2020
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.

None yet

4 participants