Skip to content

Conversation

sfriesel
Copy link
Collaborator

Buffer (the package) is only used by browserify (a dev dependency) which pulls in
its own version of buffer. As a result, this change has no effect on
the output produced by browserify.

This reduces the number of direct and indirect dependencies from 4 to 1.

Also bring package-lock.json up to date.

sfriesel added 3 commits May 10, 2020 03:12
Buffer is only used by browserify (a dev dependency) which pulls in
its own version of buffer. As a result, this change has no effect on
the output produced by browserify.
@regular
Copy link
Owner

regular commented May 13, 2020

hm, ... from the Readme:

When browserified, the stream emits instances of feross/buffer instead of raw Uint8Arrays to have a consistent API across browsers and Node.

@sfriesel

Buffer (the package) is only used by browserify

True, It isn't explicitly required by index.js. However, browserify detects the use of Buffer and will inject a Buffer implementation into its output. It will provide one if needed. IIRC, the explicit dependency on feross/buffer is here to make browserify use this one instead of the one that comes with browserify, because, historically, there has been a problem with the latter. (I think at one point it simply was a beefed-up Uint8Array)

It is important to note that we do not control the version of browserify that will be used to bundle unbzip2-stream into an application bundle. Instead this is controlled by the application's build environment. Removing the explicit dependency on buffer strips us of the opportunity to control what version of buffer is ending up being used in our module. This introduces the risk of breakage.

To put it differently: we have an actual dependency on buffer (not a dev dependency). To rely on resolving this actual dependency via a dev dependency might work in the case where our explicit dev dependency is being used in the build. (that's the case if you browserify this module on its own). However, this is not the case for applications (or other dependents), because they'll ignore our dev dependency and use their own tool chain instead.

I lean towards not merging this PR, because I don't think it's in the interest of downstream application maintainers. What do you think?

@sfriesel
Copy link
Collaborator Author

sfriesel commented May 13, 2020

(I think at one point it simply was a beefed-up Uint8Array)

In the latest version, feross/buffer is still just a beefed up Uint8Array. But let's say buffer < 3 was not sufficiently beefy because it did not pass the nodejs Buffer testsuite at the time. (unbzip2-stream 1.0 required buffer 3).

Browserify 7.1.0 and 8.0.0 pulled in buffer 3 and were released in December 2014. This predates the 1.0 release of unbzip2-stream, which had a devDependency on browserify 8.1.

It is important to note that we do not control the version of browserify that will be used to bundle unbzip2-stream into an application bundle. Instead this is controlled by the application's build environment.

We do not control the version in that case but I think we can reasonably expect the version that has been required since 1.0.0.

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.

2 participants