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

browserify build #74

Closed
wants to merge 18 commits into
base: two
from

Conversation

Projects
None yet
3 participants
@calvinmetcalf
Contributor

calvinmetcalf commented Oct 13, 2013

first stab at it pretty much an updated version of the one currently up there, need to get the tests passing in node (browser works) which may be a challenge for me as I don't really know quint well (as opposed to mocha).

@Stuk

This comment has been minimized.

Show comment
Hide comment
@Stuk

Stuk Oct 15, 2013

Owner

Looks good. I need to play around with it locally.

Owner

Stuk commented Oct 15, 2013

Looks good. I need to play around with it locally.

@calvinmetcalf

This comment has been minimized.

Show comment
Hide comment
@calvinmetcalf

calvinmetcalf Oct 15, 2013

Contributor

a couple notes:

  • I made you an author on NPM
  • browserify adds a fake Buffer shim in so if you ever test for that you have to also test for process.browser which browserify sets to true in browser builds.
Contributor

calvinmetcalf commented Oct 15, 2013

a couple notes:

  • I made you an author on NPM
  • browserify adds a fake Buffer shim in so if you ever test for that you have to also test for process.browser which browserify sets to true in browser builds.
@dduponchel

This comment has been minimized.

Show comment
Hide comment
@dduponchel

dduponchel Oct 15, 2013

Collaborator

I started playing with this pull request and here are some thoughts :

  • whoa, that's cool !
  • now that we have a proper build chain, we should be able to remove the copy/paste of the inflate/deflate code and add a npm dependency to https://github.com/imaya/zlib.js
  • do we really need the content of the dist/ folder ?
  • the generated file does not pass the unit tests on IE 6-9. I checked some of the errors and they come from updated code : a switch/case transformed into a array.indexOf (doesn't exist in IE <= 8) or a new call to new Uint8Array(input) (doesn't exist in IE <= 9)
Collaborator

dduponchel commented Oct 15, 2013

I started playing with this pull request and here are some thoughts :

  • whoa, that's cool !
  • now that we have a proper build chain, we should be able to remove the copy/paste of the inflate/deflate code and add a npm dependency to https://github.com/imaya/zlib.js
  • do we really need the content of the dist/ folder ?
  • the generated file does not pass the unit tests on IE 6-9. I checked some of the errors and they come from updated code : a switch/case transformed into a array.indexOf (doesn't exist in IE <= 8) or a new call to new Uint8Array(input) (doesn't exist in IE <= 9)
@calvinmetcalf

This comment has been minimized.

Show comment
Hide comment
@calvinmetcalf

calvinmetcalf Oct 15, 2013

Contributor
  • it doesn't look like zlib is in NPM nor is the package.json set up for that so that wouldn't be as easy as it could be
  • yes we probably need it in some form to put into package managers and bower and component both use github so committing that folder is helpful with that.
  • my bad, the my original modularization one targeted newer browsers so I can go back and fix those.
Contributor

calvinmetcalf commented Oct 15, 2013

  • it doesn't look like zlib is in NPM nor is the package.json set up for that so that wouldn't be as easy as it could be
  • yes we probably need it in some form to put into package managers and bower and component both use github so committing that folder is helpful with that.
  • my bad, the my original modularization one targeted newer browsers so I can go back and fix those.
@dduponchel

This comment has been minimized.

Show comment
Hide comment
@dduponchel

dduponchel Oct 17, 2013

Collaborator

zlib.js has a bin/ folder containing the generated files. I tested with

"dependencies" : {
  "zlib.js" : "git://github.com/imaya/zlib.js#0.1.7"
}

and in deflate.js I added var Zlib = require('zlibjs/bin/rawdeflate.min').Zlib; (same with inflate). The require line is a bit awkward but I think the result (1800 LoC removed and a clean dependency) is worth it.

Regarding the dist/ folder, I clearly see the advantage of having the generated files (I just used them with zlib.js) but I also see some big disadvantages. On the master branch they need to be up to date which means that either :

  • we commit the updated dist/ after each pull request
  • we manually merge each pull request to update it
  • we put the burden on the contributors
  • I forgot something and it isn't as bad as it seems to be

This could also generate merge conflicts in the minified file.

We could create an other branch to contains the dist/ folder (but some package managers use the tags so that may not be wise) or we could create a new repository jszip-package to contains only builds, documentation and <package-manager>.json.

Thoughts ?

Collaborator

dduponchel commented Oct 17, 2013

zlib.js has a bin/ folder containing the generated files. I tested with

"dependencies" : {
  "zlib.js" : "git://github.com/imaya/zlib.js#0.1.7"
}

and in deflate.js I added var Zlib = require('zlibjs/bin/rawdeflate.min').Zlib; (same with inflate). The require line is a bit awkward but I think the result (1800 LoC removed and a clean dependency) is worth it.

Regarding the dist/ folder, I clearly see the advantage of having the generated files (I just used them with zlib.js) but I also see some big disadvantages. On the master branch they need to be up to date which means that either :

  • we commit the updated dist/ after each pull request
  • we manually merge each pull request to update it
  • we put the burden on the contributors
  • I forgot something and it isn't as bad as it seems to be

This could also generate merge conflicts in the minified file.

We could create an other branch to contains the dist/ folder (but some package managers use the tags so that may not be wise) or we could create a new repository jszip-package to contains only builds, documentation and <package-manager>.json.

Thoughts ?

@calvinmetcalf

This comment has been minimized.

Show comment
Hide comment
@calvinmetcalf

calvinmetcalf Oct 17, 2013

Contributor

first on the dist front the usual route in my experience is some combination of 1 and 2. The key is to make it clear that people who pull are not to commit them and then it ends up not being much of a deal.

for zlib it occurs to me we could just use the zlib which browserify adds if you do var zlib = require('zlib') which is a wrapper for @imaya's anyway

Contributor

calvinmetcalf commented Oct 17, 2013

first on the dist front the usual route in my experience is some combination of 1 and 2. The key is to make it clear that people who pull are not to commit them and then it ends up not being much of a deal.

for zlib it occurs to me we could just use the zlib which browserify adds if you do var zlib = require('zlib') which is a wrapper for @imaya's anyway

compressInputType: null,
uncompressInputType: null
};
exports.DEFLATE = require('./flate');

This comment has been minimized.

@Stuk

Stuk Oct 20, 2013

Owner

Requiring directories with an index.js file is not supported by other CommonJS module loaders like Mr. This should be changed to ./flate/index or rename "lib/flate/index.js" to "lib/flate.js".

@Stuk

Stuk Oct 20, 2013

Owner

Requiring directories with an index.js file is not supported by other CommonJS module loaders like Mr. This should be changed to ./flate/index or rename "lib/flate/index.js" to "lib/flate.js".

@Stuk

This comment has been minimized.

Show comment
Hide comment
@Stuk

Stuk Oct 20, 2013

Owner

One thing I was considering is allowing the deflate/inflate and load modules to be optionally loaded to ease the file size when using it in the browser. Something along the lines of:

// everything
var JSZip = require("jszip");

// optionally loading
var JSZip = require("jszip/base")

JSZip.addCompression(require("jszip/deflate"));
JSZip.addCompression(require("jszip/inflate"));
JSZip.prototype.load = require("jszip/load");

Any thoughts? These are the filesizes:

# before
173K Oct 20 11:19 jszip.js
 69K Oct 20 11:19 jszip.min.js
# after
149K Oct 20 11:57 jszip.js
 60K Oct 20 11:57 jszip.min.js
Owner

Stuk commented Oct 20, 2013

One thing I was considering is allowing the deflate/inflate and load modules to be optionally loaded to ease the file size when using it in the browser. Something along the lines of:

// everything
var JSZip = require("jszip");

// optionally loading
var JSZip = require("jszip/base")

JSZip.addCompression(require("jszip/deflate"));
JSZip.addCompression(require("jszip/inflate"));
JSZip.prototype.load = require("jszip/load");

Any thoughts? These are the filesizes:

# before
173K Oct 20 11:19 jszip.js
 69K Oct 20 11:19 jszip.min.js
# after
149K Oct 20 11:57 jszip.js
 60K Oct 20 11:57 jszip.min.js
@Stuk

This comment has been minimized.

Show comment
Hide comment
@Stuk

Stuk Oct 20, 2013

Owner

Amusingly It appears that browserify is shimming Buffer: https://github.com/calvinmetcalf/jszip/blob/7e47cfd75d4cbdcdab6721c80a9a80ed22280c39/dist/jszip.js#L3189. Any ideas if this can be disabled?

Owner

Stuk commented Oct 20, 2013

Amusingly It appears that browserify is shimming Buffer: https://github.com/calvinmetcalf/jszip/blob/7e47cfd75d4cbdcdab6721c80a9a80ed22280c39/dist/jszip.js#L3189. Any ideas if this can be disabled?

@calvinmetcalf

This comment has been minimized.

Show comment
Hide comment
@calvinmetcalf

calvinmetcalf Oct 20, 2013

Contributor

this has come up in daleharvey/pouchdb#921, we'd have to make sure Buffer was only ever called from some node only files and then exclude that from the build OR as the buffer shim doesn't use typed arrays, use the NodeReader as a fallback for things that don't have arrays buffers.

as for size things, gzipped in this current config you save 4k by excluding deflate and inflate (so 2k each) and you save 1.6k by excluding load. it would make more sense if there were multiple algorithms that might be used, but at the moment it comes down to saving ~2k if your only going one way (deflate or inflate) and then saving ~1.6k if your not actual loading somebody else's zipfile? not sure what the load part actually does. In my experience custom builds don't get used much and end up not being used a ton, it seems like not much of a gain for a lot of extra hasle for people.

Contributor

calvinmetcalf commented Oct 20, 2013

this has come up in daleharvey/pouchdb#921, we'd have to make sure Buffer was only ever called from some node only files and then exclude that from the build OR as the buffer shim doesn't use typed arrays, use the NodeReader as a fallback for things that don't have arrays buffers.

as for size things, gzipped in this current config you save 4k by excluding deflate and inflate (so 2k each) and you save 1.6k by excluding load. it would make more sense if there were multiple algorithms that might be used, but at the moment it comes down to saving ~2k if your only going one way (deflate or inflate) and then saving ~1.6k if your not actual loading somebody else's zipfile? not sure what the load part actually does. In my experience custom builds don't get used much and end up not being used a ton, it seems like not much of a gain for a lot of extra hasle for people.

@dduponchel

This comment has been minimized.

Show comment
Hide comment
@dduponchel

dduponchel Oct 30, 2013

Collaborator

I'm still don't think that commiting generated files is a good idea but this seems to be the easiest path. As you pointed out @calvinmetcalf, this is done by a lot of js devs (for each commit or for each release). Other repositories use a custom tarball with npm (I've never published any package on npm but that seems possible) or have a separated repository (angularjs but that's the only one I found). With other package managers (see #19) adding generated files may be the easiest solution.

The zlib wrapper won't work for us : the nodejs API is asynchronous while JSZip has a synchronous API (but that would have been nice).

The Buffer shim adds a lot of code :

# on master : (concatenate all js files)
 97224 jszip.all.js
# with browserify
177477 jszip.js
# with detectGlobals : false (without the Buffer shim)
109655 jszip.js

I think we should set detectGlobals : false in the browserify configuration :-) (and remove the use of process.browser)

Collaborator

dduponchel commented Oct 30, 2013

I'm still don't think that commiting generated files is a good idea but this seems to be the easiest path. As you pointed out @calvinmetcalf, this is done by a lot of js devs (for each commit or for each release). Other repositories use a custom tarball with npm (I've never published any package on npm but that seems possible) or have a separated repository (angularjs but that's the only one I found). With other package managers (see #19) adding generated files may be the easiest solution.

The zlib wrapper won't work for us : the nodejs API is asynchronous while JSZip has a synchronous API (but that would have been nice).

The Buffer shim adds a lot of code :

# on master : (concatenate all js files)
 97224 jszip.all.js
# with browserify
177477 jszip.js
# with detectGlobals : false (without the Buffer shim)
109655 jszip.js

I think we should set detectGlobals : false in the browserify configuration :-) (and remove the use of process.browser)

@calvinmetcalf

This comment has been minimized.

Show comment
Hide comment
@calvinmetcalf

calvinmetcalf Oct 31, 2013

Contributor

your right about zlib my bad. I'm somewhat hesitant about relying on the external zlib.js as it brings in some other stuff but it's still better then bringing it with.

I believe we can do better then detectGlobals:false what we can do is use !process.browser as a proxy for buffers being available, make sure all the buffer specific code is in a few files, and then use ignore to exclude those files from the build all together, that way not only no buffer shim but also no node specific code at all.

Contributor

calvinmetcalf commented Oct 31, 2013

your right about zlib my bad. I'm somewhat hesitant about relying on the external zlib.js as it brings in some other stuff but it's still better then bringing it with.

I believe we can do better then detectGlobals:false what we can do is use !process.browser as a proxy for buffers being available, make sure all the buffer specific code is in a few files, and then use ignore to exclude those files from the build all together, that way not only no buffer shim but also no node specific code at all.

@calvinmetcalf

This comment has been minimized.

Show comment
Hide comment
@calvinmetcalf

calvinmetcalf Oct 31, 2013

Contributor

ok used @dduponchel's method for moving zlib out of here, also moved new Buffer and Buffer.isBuffer into a new module that we can then ignore in the build process (along with nodeBufferReader)

before

Original: 159374 bytes.
Minified: 71022 bytes.
Gzipped:  16075 bytes.

now

Original: 93148 bytes.
Minified: 44675 bytes.
Gzipped:  10773 bytes.
Contributor

calvinmetcalf commented Oct 31, 2013

ok used @dduponchel's method for moving zlib out of here, also moved new Buffer and Buffer.isBuffer into a new module that we can then ignore in the build process (along with nodeBufferReader)

before

Original: 159374 bytes.
Minified: 71022 bytes.
Gzipped:  16075 bytes.

now

Original: 93148 bytes.
Minified: 44675 bytes.
Gzipped:  10773 bytes.
@dduponchel

This comment has been minimized.

Show comment
Hide comment
@dduponchel

dduponchel Oct 31, 2013

Collaborator

Nice !

Collaborator

dduponchel commented Oct 31, 2013

Nice !

Calvin Metcalf added some commits Oct 31, 2013

@calvinmetcalf

This comment has been minimized.

Show comment
Hide comment
@calvinmetcalf

calvinmetcalf Oct 31, 2013

Contributor

308660e deals with the issue that @Stuk mentioned of certain module loaders not being able to deal with index.js files for modules

Contributor

calvinmetcalf commented Oct 31, 2013

308660e deals with the issue that @Stuk mentioned of certain module loaders not being able to deal with index.js files for modules

@dduponchel

This comment has been minimized.

Show comment
Hide comment
@dduponchel

dduponchel Nov 19, 2013

Collaborator

Sorry for the lack of update !
I still have issues on IE (will comment on the offending lines) but this pull request looks good.
@calvinmetcalf could you rebase the branch ? The branch two has been rebased and leaved your branch with duplicated commits.
@Stuk have you other comments ?
Once this pull request is merged I think we should release a new version : this will allow bower to have a tag with everything needed (still don't know if this should be a v2.0.1 or a v2.1.0).

Collaborator

dduponchel commented Nov 19, 2013

Sorry for the lack of update !
I still have issues on IE (will comment on the offending lines) but this pull request looks good.
@calvinmetcalf could you rebase the branch ? The branch two has been rebased and leaved your branch with duplicated commits.
@Stuk have you other comments ?
Once this pull request is merged I think we should release a new version : this will allow bower to have a tag with everything needed (still don't know if this should be a v2.0.1 or a v2.1.0).

@dduponchel

This comment has been minimized.

Show comment
Hide comment
@dduponchel

dduponchel Nov 19, 2013

The attributes uncompressInputType and compressInputType are required : they are used to convert data into the right format. Without them, things work by accident : the library gets an Uint8Array in the unit tests because ArrayBuffers are automatically converted. In IE, the conversion String -> Array doesn't happen and the tests break :-)

dduponchel commented on lib/flate/index.js in a56ecf6 Nov 19, 2013

The attributes uncompressInputType and compressInputType are required : they are used to convert data into the right format. Without them, things work by accident : the library gets an Uint8Array in the unit tests because ArrayBuffers are automatically converted. In IE, the conversion String -> Array doesn't happen and the tests break :-)

@dduponchel

This comment has been minimized.

Show comment
Hide comment
@dduponchel

dduponchel Jan 15, 2014

Collaborator

Merged into master.

Collaborator

dduponchel commented Jan 15, 2014

Merged into master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment