variable _ should be renamed to avoid conflicts

BorisMoore / jquery-tmpl

The original official jQuery Templates plugin. This project was maintained by the jQuery team as an official jQuery plugin. It is no longer in active development, and has been superseded by JsRender.

3.23k stars 1.01k forks source link

variable _ should be renamed to avoid conflicts #10

Closed dvv closed 13 years ago

dvv commented 14 years ago

Hi! Templates uses internally a variable called _ and I couldn't find any warning about the user SHOULDN'T EVER USE this name in his code. I'd suggest to rename _ to something more obscure such as ___$$$___ to ensure no interference with user code is done.

TIA, --Vladimir

P.S. BTW, almost all templating engines have such an issue. --Vladimir

BorisMoore commented 14 years ago

That variable is within a closure, so will not collide with global variables. But it will collide with a field name on data, so in that respect you are right. Did you have a scenario where you had a collision?

dvv commented 14 years ago

Sure. Due to with() _ can be brought in by the context. E.g., sammy (http://code.quirkey.com/sammy) has a rendering plugin which passes context which is merged from many pieces, starting from Sammy.Application with its helpers. I used to use _ as a shortcut to a simple i18n function, and got confused. Also, underscore defines _.

BorisMoore commented 14 years ago

Yes, so it is the field name issue on data (due to with()) I mention above. Thanks, I agree we should look at changing this...

dvv commented 14 years ago

Frankly, I'd get rid of with(). You see, when user is not defined in the context passed, with(context){var a = user;} throws, but var a = context.user; behaves more gently and more right. imho, at least

BorisMoore commented 14 years ago

In most cases that is not an issue, since one does not add code within the scope of the with. ${user} will work fine even if user is not defined on some data items. (The template tags deal with 'undefined' without throwing...

dvv commented 14 years ago

Right. I opened instead issue #11 concerning ReferenceError

givankin commented 13 years ago

Hi, I reproduced variable collision with global _.

In short, if there is a global and an {{each [,]}} or {{each {length:3} }} or a similar construction (which I used for iterating N times within a template: imagine paginator), script dies with ".push is not a function" error.

Here are tests that reproduce the issue:

No global _. Everything is fine.
Introducing underscore.js. See the error.
Using underscore's noConflict method plus "delete window._". I'd use this happily if it didn't break IE < 8.

dvv commented 13 years ago

I keep thinking pure internal template variables should not clash with anything, documentcloud's _ being an ordinary common case.

givankin commented 13 years ago

@dvv Do you understand how and why that clash occurs? _ in jquery.tmpl.js seems to be really pure internal.

dvv commented 13 years ago

Sure. Being pure internal, it could be named as _____________ in hope it'd never clash with anything. Instead, they chose simple _, which is already chosen by many.

givankin commented 13 years ago

That's right, but templates' is within a closure, so as far as I understand it shouldn't collide with global . What am I missing?

dvv commented 13 years ago

Please, look at BorisMoore's comments above. The point is in "who wins" during lookup for a variable, and I presume with(...){...}, which is used in templating engine, breaks some assumptions.

BTW, any function in JS is a closure, and it captures the context in which it is defined. I've discovered it only recently. I'd recommend to read http://dmitrysoshnikov.com/ecmascript/chapter-5-functions/

BorisMoore commented 13 years ago

The collision with global is caused in abbakym's sample because he is using {{each [,]}} - which is implemented by each([,], function($index, $value){with(this)... The $value is undefined, so the this pointer falls back to the document object. We don't support {{each [,]}}! {{each []}} or {{each [a,b]}} where a and b are defined will work fine!

givankin commented 13 years ago

Thanks for explanation! If your lack of support for {{each [,]}} was somehow documented I suppose it would save developers some time and nerves ;-)

And yes, if there is no _ in global scope, {{each [,]}} works as expected, so I think you can't say that it is truly not supported. Maybe there is a simple workaround for this? What's the reason for not supporting this syntax?

BorisMoore commented 13 years ago

Well at any rate, we will fix the _ issue, so at that point there will be no collision. This issue is open to track that...

As you say, [,] is otherwise supported, though it will likely not be a common scenario

BorisMoore commented 13 years ago

esuslo commented 13 years ago

Boris, On behalf of all beautiful ladies in Copenhagen, I am asking you to prioritize the _ issue, as this is the only reason our project doesn't work correctly on IE, thus preventing hot girls to meet nice guys, as you are ;)

Eugene

BorisMoore commented 13 years ago

OK, fixed in https://github.com/jquery/jquery-tmpl/commit/fe9d7e4fe7659fdcaae686a391597ff6dd730cbd. Note that I have set the variable used in buildTmplFn to , as a variable that is unlikely to exist as a global, or as a field name on data. I could have chosen a longer one, such as __$9, or whatever, but there is a cost in extra code size of jquery.tmpl.js, and also on compiled template size...