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

make the new operator optional before the JSZip constructor #93

Merged
merged 3 commits into from
Feb 13, 2014
Merged

make the new operator optional before the JSZip constructor #93

merged 3 commits into from
Feb 13, 2014

Conversation

Mithgol
Copy link
Contributor

@Mithgol Mithgol commented Feb 11, 2014

As @qlqllu has reported, an attempt to shorten JSZip call (giving it the form var jsZIP = new require('jszip')()) fails.

The failure's cause is a well-known JavaScript's operator precedence (the operator new has higher precedence than a function call — even if Node's require is that function). However, frankly speaking, that precedence in this case looks counter-intuitive to me as well.

This case reminded me of John Resig's self-calling constructor pattern (given in Resig's blog entry Simple “Class” Instantiation in 2007). Nobody uses new $ with Resig's jQuery and, I think, JSZip might follow jQuery's example and benefit from it.

This pull request makes the new operator optional before JSZip's constructor. Programmers might continue using new or omit it, depending on their inner personal preferences:

  • not typing four characters (new + space) might feel more RAD and less boilerplate code;
  • typing new might serve as a reminder that a constructor call is ahead;
  • using someZIP = require('jszip')() instead of someZIP = new (require('jszip'))() means less brackets;
  • etc.

(Unlike the example in Resig's blog, this pull request refrains from using arguments.callee because the use of that property is believed to slow down the engine.)

This change introduces a new backwards-compatible feature. An npm publish of a new minor version is recommended.

@Mithgol Mithgol mentioned this pull request Feb 11, 2014
@dduponchel
Copy link
Collaborator

@Mithgol Nice ! Could you add an unit test for this constructor ? We already have one for current constructor. And I agree with you, this leads to a new minor version.

@Stuk should I prepare a v2.1.1 (with only #91) first / in parallel ? I think #91 is a bit urgent (as the installation fails behind a restrictive firewall) but we could wait to have more stuff for the v2.2.0. What do you think of it ?

@Stuk
Copy link
Owner

Stuk commented Feb 12, 2014

@dduponchel a quick v2.1.1 with #91 sounds good to me

@dduponchel
Copy link
Collaborator

I'm preparing it !

@qlqllu
Copy link

qlqllu commented Feb 13, 2014

I can see the version 2.1.1 in npm registry. I have tested it, it works great!
Thanks!

@Mithgol
Copy link
Contributor Author

Mithgol commented Feb 13, 2014

I've just added a test; it passed on Travis CI.

@@ -59,6 +59,9 @@ test("JSZip", function(){

var zip = new JSZip();
ok(zip, "Constructor works");

var zipNoNew = JSZip();
ok(zipNoNew.file, "Constructor adds `new` before itself where necessary");
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these tests would be more explicit if changed to ok(zip instanceof JSZip, ...). Would you mind doing that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Stuk added a commit that referenced this pull request Feb 13, 2014
make the `new` operator optional before the `JSZip` constructor
@Stuk Stuk merged commit a336db3 into Stuk:master Feb 13, 2014
@Mithgol Mithgol mentioned this pull request Feb 21, 2014
dduponchel added a commit to dduponchel/jszip that referenced this pull request Feb 22, 2014
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.

None yet

4 participants