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

Allow node.js Buffers in browser, since browserify provides them automatically. Fixes #139 #140

Merged
merged 1 commit into from
Jul 23, 2014

Conversation

humphd
Copy link
Contributor

@humphd humphd commented Jun 7, 2014

This allowed me to do what I needed, namely, use Buffers (i.e., augmented Uint8Arrays created by browserify) when creating and extracting files from zip archives.

@dduponchel
Copy link
Collaborator

Checking the availability of Buffer seems good !

The people using the generated files (in dist/) of JSZip may have an issue : the browserify implementation uses instanceof for Buffer.isBuffer.

In a browser, if the application does not use browserify and but uses browersified libraries (including JSZip), we will get the following state :

  • JSZip.support.nodebuffer will be true, but :
  • JSZip won't accept buffers from other libraries (because each lib has its own Buffer class and instanceof won't work)
  • JSZip will generate buffers that don't pass the isBuffer check

The above issue could be solved (I think) by just disabling the Buffer polyfill in our browserify build and letting the environment provide (or not) a Buffer. What do you think ?

@Stuk
Copy link
Owner

Stuk commented Jun 7, 2014

Yep, David is correct. This library can be used with other browser CommonJS module loaders like Mr, which don't provide a shim. We need to make sure that that case is handled too.

@humphd
Copy link
Contributor Author

humphd commented Jun 9, 2014

For the most part, this can remain transparent, and users can do what makes sense to them. Your generate already has parametrized return types, so if you don't want Buffer, you're not forcing anyone to use it--what I'm suggesting will just make it possible if you do, since your code is willing and able to use Buffer if they aren't turned off through type checks. The load function is similar, in that you can inspect what a caller gives you at runtime: if it's a Buffer then use it, if it's a Uint8Array, use that, etc.

If you really want to provide explicit support for this, one way to solve it is to expose Buffer on JSZip somehow, JSZip.Buffer or JSZip.support.Buffer or JSZip.support.NodeBuffer etc. You already have a basic setup for this internally, since you wrap Buffer in nodeBuffer.js. I have the same issue in Filer, where I'm planning to do Filer.Buffer, which just gives access to Buffer (either node.js or browserify's).

@dduponchel
Copy link
Collaborator

I agree with you, I just want to avoid a situation where JSZip forces an implementation of Buffer that doesn't play well outside of JSZip.
Exposing Buffer to let the user specify the implementation is a good idea but I think that excluding the files lib/nodeBuffer.js and lib/nodeBufferReader.js from our browserify build is better for now.
Adding them in a ignore option in our Gruntfile.js should be ok for anyone, no ?

We will need to exclude files from the generated files in the future anyway : I'm working on async methods and I take advantage of the native async libraries if they are available (not to provide features, just a speed up). Including everything in the bundled files almost double the size.

@dduponchel dduponchel mentioned this pull request Jun 12, 2014
@dduponchel
Copy link
Collaborator

I've prepared a commit here that should satisfy everyone.
@humphd is this OK for you too ?

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.

3 participants