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

Full zlib module support #721

Closed
wants to merge 1 commit into from
Closed

Conversation

devongovett
Copy link
Member

I implemented a replacement module for the current zlib module being used by browserify that supports the full Node zlib API. It uses the actual JS zlib code from Node, and passes Node's zlib tests by basically implementing the C++ binding that Node uses on top of pako, which is a direct port of zlib to JS. This means that the full API is implemented (including streaming, async, sync, etc.), the API and behavior exactly matches Node, and because pako is a direct port of zlib, the output is binary identical to the output produced by Node's zlib module. Also, pako is way faster than the zlib library being used by the current module (see benchmarks on their repo compared to imaya). The only things not implemented yet are the params method and the dictionary option because pako doesn't have the underlying methods for those yet, but I hope to write patches for those soon. Let me know what you think!

@ghost
Copy link

ghost commented Apr 8, 2014

This looks great. One small issue:

$ browserify -r zlib
Error: Cannot find module '/home/substack/projects/node-browserify/node_modules/browserify-zlib/brfs' from '/home/substack/projects/node-browserify'

This is because browserify-zlib has:

  "browserify": {
    "transform": [
      "brfs"
    ]
  },

in the package.json of the root project, but brfs is only a devDependency because it's only used in the tests. Just move that transform field into a package.json in test/ with:

{
  "browserify": {
    "transform": [
      "brfs"
    ]
  }
}

and everything should be fine.

@ghost
Copy link

ghost commented Apr 8, 2014

I sent a pull request upstream that should fix this issue: browserify/browserify-zlib#1

@devongovett
Copy link
Member Author

Ah, good catch! Should be fixed in browserify-zlib@0.1.2.

@devongovett
Copy link
Member Author

Whoops, haha, nice timing. 😄

@ghost
Copy link

ghost commented Apr 8, 2014

Merged in 3.39.0. Thanks for the patch!

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant