Opened 11 years ago

Closed 10 years ago

#9067 closed defect (fixed)

"this" shouldn't be referenced outside of "classes"

Reported by: bill Owned by: Eugene Lazutkin
Priority: high Milestone: 1.4
Component: Core Version: 1.3.0
Keywords: Cc: James Burke
Blocked By: Blocking:

Description

The dojo core code references "this" inside of normal functions. Probably this is for supporting multiple versions of dojo running on the same page?

For example:

every: function(/*Array|String*/arr, /*Function|String*/callback, /*Object?*/thisObject){
	...
	return this._everyOrSome(true, arr, callback, thisObject); // Boolean
},

It could be considered a bug though. For example, the following code won't work:

var every = dojo.every;
every([1,2,3], function(x){ console.log(x); return true; });

It returns an error that:

TypeError: this._everyOrSome is not a function

Change History (15)

comment:1 Changed 11 years ago by bill

Owner: changed from anonymous to Eugene Lazutkin

James says that this is legacy code for assigning dojo to another object, and came be removed.

And Eugene volunteers to fix it... you'll need to scour the codebase for invalid references to this, I'm not sure how many there are but I remember seeing more than just this one (no pun intended).

comment:2 Changed 11 years ago by Eugene Lazutkin

Status: newassigned

#9114 --- yet one more argument for keeping namespaces thisless.

comment:3 Changed 11 years ago by Eugene Lazutkin

Milestone: tbdfuture

comment:4 Changed 11 years ago by Eugene Lazutkin

Priority: normalhigh

comment:5 Changed 10 years ago by Eugene Lazutkin

(In [17942]) Replacing "this" with "dojo", !strict, refs #9067.

comment:6 Changed 10 years ago by Eugene Lazutkin

(In [17943]) Replacing "this" with "dojo", !strict, refs #9067.

comment:7 Changed 10 years ago by Eugene Lazutkin

(In [17944]) Making _everyOrSome a private function, !strict, refs #9067.

comment:8 Changed 10 years ago by Eugene Lazutkin

(In [17945]) Making _everyOrSome a private function, !strict, refs #9067.

comment:9 Changed 10 years ago by Eugene Lazutkin

Milestone: future1.4
Resolution: fixed
Status: assignedclosed

I couldn't find any more uses of "this" as a module reference in the Base. Please reopen, if you find anything.

comment:10 Changed 10 years ago by bill

Resolution: fixed
Status: closedreopened

There are a lot of this references in the hostenv* and loader* files but I think they are OK.

However, I think I see some invalid references in:

  • Color.js
  • Event.js (_stopPropogation etc, maybe these are ok?)

There's also some references in the parser but I'll deal with those separately.

comment:11 Changed 10 years ago by Eugene Lazutkin

I saw the "this" use in Color.js and event.js, but it appears to be proper: they both define classes and objects, and use this to reference an instance.

But I didn't inspect the whole _loader directory --- good catch! And I'll double-check both Color.js and event.js.

comment:12 Changed 10 years ago by Adam Peller

Cc: James Burke added

Referencing 'this' from global scope gets the global object. This 'feature' is regretted and often blamed for security problems and is one of the things caja/adsafe seeks to prevent. IIRC, ES5 strict mode changes this to be null in this situation. Whether or not we plan to support strict mode is another question (I'd imagine we should) but it's something we should probably avoid doing in the bootstrap unless we can justify it.

comment:13 Changed 10 years ago by James Burke

I think "this" was mostly just used so that you could change what the dojo object was defined to be, and still have the functions work. However, that is probably of marginal benefit vs. not being able to need to call a dojo method with a proper this object, as well as the caja concerns.

dojo.global = this; is probably the only problematic "this" reference. Maybe that is something we need to push to the hostenvs. But then I'm not sure we need to be completely militant about it either. If it works in the hostenvs easily, we can go for that, otherwise leave it as is.

comment:14 Changed 10 years ago by Eugene Lazutkin

(In [19497]) Eliminating "this" from the loader modules, !strict, refs #9067.

comment:15 Changed 10 years ago by Eugene Lazutkin

Resolution: fixed
Status: reopenedclosed

Combed _loader/, found some valid cases of using this:

  • When dojo is being set up, this === window.
  • There are some objects, which use this in their constructor/methods.

The rest was converted to dojo.

A side note: in some files d is not used as a short-cut to dojo => it may prevent a simple shrinksafe compresion, because dojo cannot be substituted with anything shorter. But it is a minor nit.

Note: See TracTickets for help on using tickets.