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

Avoid unnecessary global variables #15

Closed
w0rm opened this issue Jan 19, 2014 · 5 comments · Fixed by #16
Closed

Avoid unnecessary global variables #15

w0rm opened this issue Jan 19, 2014 · 5 comments · Fixed by #16
Milestone

Comments

@w0rm
Copy link
Collaborator

w0rm commented Jan 19, 2014

Dragdealer creates 3 global variables (Dragdealer, Cursor, Position). While it would be better to expose only one global that is accessed from the outside of the module.

This can be easily fixed by wrapping module contents in self-executing anonymous function. Another option to consider is to use UMD pattern.

@ovidiuch
Copy link
Owner

I have this thing written on a note, you read my mind completely :). I thought about the anonymous wrapper but my last impression was that it should simply be Dragdealer, Dragdealer.Cursor, and Dragdealer.Position, change the calls when you're done and that's that.

A cool test would be somehow to check the global entities before loading the dragdealer lib, and then after, expecting only one increment in the number of keys on window. But this is pushing it a bit and you'd probably have to use an iframe for this

Feel free to push for the best option you feel Dragdealer should adopt

@w0rm
Copy link
Collaborator Author

w0rm commented Jan 19, 2014

What are your thoughts about adding optional support for AMD? I'm a big fan of requirejs, so I suggest this wrapping:

(function (root, factory) {
  if (typeof define === 'function' && define.amd) {
    // AMD. Register as an anonymous module.
    define(factory);
  } else {
    // Browser globals
    root.Dragdealer = factory();
  }
}(this, function () {
  var Dragdealer = function(){}
  var Cursor = Dragdealer.Cursor = {}
  var Position = Dragdealer.Position = {}
  return Dragdealer;
}));

@ovidiuch
Copy link
Owner

Sure, I use requirejs a lot. Wasn't aware of this wrapping but looks pretty cool to me.

Don't know if there's a need for the var Cursor = Dragdealer.Cursor = {} thing now, since Dragdealer is all you return.

Awesome stuff!

@w0rm
Copy link
Collaborator Author

w0rm commented Jan 19, 2014

var Cursor = Dragdealer.Cursor = {} will allow Dragdealer to use Cursor in local scope so the rest of the code will remain unchanged (no need to replace Cursor with Dragdealer.Cursor)

It will also allow better minification (when local variables get shorter names).

@ovidiuch
Copy link
Owner

I get that, I was thinking that we drop the Dragdealer.Cursor altogether and leave it like it is now. You think there would be some value in making them public through the Dragdealer namespace? I guess, maybe for debugging, cause I don't think other libs should rely on them through cross-dependencies :)

ovidiuch added a commit that referenced this issue Jan 19, 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 a pull request may close this issue.

2 participants