ded / klass

a utility for creating expressive classes in JavaScript
751 stars 78 forks source link

Avoid global namespace polluting #13

Closed alexstrat closed 11 years ago

alexstrat commented 12 years ago

As you might know, it's not recommanded to pollute global namespace in node.js.. Some test frameworks like Mocha fails as soon as you try to define a glabol variable..

This fix aims at preventing global namespace pollution in node.

Please merge if interested and update on npm please.

ded commented 12 years ago

that's strange. i haven't had problems with this before in Node. Plus, it shouldn't be defining a global anyhow... since it's set to module.exports

alexstrat commented 12 years ago

Yes, you're right, there is no (obvious) problem..

Indeed, defining a global variable is not a problem itself, but can lead to (see, for instance, this... yes, it's crockford, but..). That's why it's not recommended to use global variable when not needed. And thus some code quality tools (like jsLint, jsHint) and test frameworks complains when using it.

When using module.exports (or whatever module loader system), you don't need global variables anymore, which would pollute the global namespace. However, in klass, the variable klass is defined (context = this = global where global is the global namespace in node) globaly whatever the case is.

You can test it :

require('klass');
typeof global.klass //'function'
typeof klass //'function'

That is even more dangerous, in the case of klass, because :

Hope you get my point of view ! :)

ded commented 12 years ago

oh whoa. i forgot that was left there. none of my other modules are doing that... looks like i just forgot to update this one. i'll have a bit of rearrange

alexstrat commented 12 years ago

Myes.. I didn't understand for what purpose was this context.klass = klass there, since you already got this.. I think you can simply delete this line, nope ?

alexstrat commented 12 years ago

Any update here ?

0b10011 commented 11 years ago

Fixed by c33b3c8114e4243c63619772b156f480d0d8af28?

ded commented 11 years ago

@bfrohs yup.