regular / unbzip2-stream

streaming unbzip2 implementatio in pure javascript for node and browsers
Other
29 stars 23 forks source link

Remove dependency on buffer #35

Open sfriesel opened 4 years ago

sfriesel commented 4 years ago

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.

regular commented 4 years ago

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 commented 4 years ago

(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.