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

Don't include the Buffer polyfill in dist/ files. #158

Merged
merged 3 commits into from
Jul 23, 2014

Conversation

dduponchel
Copy link
Collaborator

In a browser, there are multiple implementations of nodejs' Buffer. If we let browserify includes its own, we won't play nicely with the others (it uses instanceof and doesn't accept other implementations).
With this patch, we don't include any implementation but we let the environment provides (or not) the Buffer.

This pull request includes #140 (fixing #139 too) and the fix I prepared. I got no response from @humphd so I assume he's ok with it :)

humphd and others added 3 commits June 6, 2014 21:25
In a browser, there are multiple implementations of nodejs' Buffer. If
we let browserify includes its own, we won't play nicely with the others
(it uses `instanceof` and doesn't accept other implementations).
With this patch, we don't include any implementation but we let the
environment provides (or not) the Buffer.
@humphd
Copy link
Contributor

humphd commented Jul 21, 2014

Ah, lost this in my inbox while on holidays. I'll test your patch and make sure it works for my case. Thanks for this, I'll reply again soon.

@Stuk
Copy link
Owner

Stuk commented Jul 22, 2014

Looks good.

The nodeBuffer.js and nodeBufferReader.js files are included in the bundle after this PR. Is that intended?

@dduponchel
Copy link
Collaborator Author

I'll add the files in the ignore option. The difference in the generated file is quite small :

// with the nodeBuffer*.js files
File dist/jszip.min.js created: 286.65 kB → 75.23 kB → 16.5 kB (gzip)
// without them
File dist/jszip.min.js created: 285.83 kB → 74.74 kB → 16.47 kB (gzip)

@Stuk
Copy link
Owner

Stuk commented Jul 22, 2014

Yeah, not a size issue, just wanted to make sure it wasn't going to have some unintended consequences.

@dduponchel
Copy link
Collaborator Author

I am wondering if we should keep these files in the browserify bundle. In a browser with a Buffer shim, the new check (support.nodebuffer = typeof Buffer !== "undefined") will be true and JSZip will need these files.

@dduponchel
Copy link
Collaborator Author

I tested with a Buffer shim (a standalone browserify build of https://github.com/feross/buffer) :

  • without these files, I get a lot of nodeBuffer.test is not a function errors
  • with these files, the unit tests pass (and JSZip.support.nodebuffer is true)

I'll remove the last commit :)
edit : done

Stuk added a commit that referenced this pull request Jul 23, 2014
Don't include the Buffer polyfill in dist/ files.
@Stuk Stuk merged commit 94e4db2 into Stuk:master Jul 23, 2014
@dduponchel dduponchel deleted the issue_140 branch March 24, 2016 22:05
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