"this" in AMD callback

[Bill Keese]

RequireJS recently implemented a change where this inside the define() callback points to module.exports.   This breaks existing code such as dojo which expected this to point to the global object (essentially the window Object on browsers, and whatever the global object is in Node.js).   For example:

MyModule.js:

define([], function(){

    return this.foo;

});

and then some user code:

foo = 123;

require(["acme/MyModule"], function(mm){

   console.log(mm.foo);    // prints 123

});

AFAIK the meaning of this inside the callback is undefined.   But any thoughts on this?   For example:

1. client code should get this from a closure, i.e. (function(){ return this; })().foo instead of just this.foo

2. RequireJS shouldn't mess with the value of this inside the callback; it should point to the global

3. there should be a module.global property that points to the global object

Thanks,

Bill

[John Hann]

For maximum compat with node modules, curl.js sets the context to module.exports when the module is AMD-wrapped CommonJS.  Otherwise, it's undefined. That seems like the right thing to do, imho. 

Honestly, nobody should ever use `this` in a global or module scope, but obviously nobody told the node devs that back when they implemented their interpretation of CommonJS. :)

-- John

Sent from planet earth

--
You received this message because you are subscribed to the Google Groups "amd-implement" group.
To unsubscribe from this group and stop receiving emails from it, send an email to amd-implemen...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Bill Keese]

I guess that means that dojo has never worked with the curl loader.   John, any ideas on how to get a reference to the global scope (or whatever its called)?  James thought that the  (function(){ return this; })() approach wouldn't work when "use strict" was set.   Judging from http://stackoverflow.com/questions/5447771/node-js-global-variables perhaps we should get it as typeof global === "undefined" ? window : global.

Some may argue that AMD modules shouldn't try to set/get global variables, but OTOH it seems OK for applications to use global variables (ex: <div onclick="myOnclickMethod();">), and AMD modules may need to access those globals.  That's the case for dojo's parser anyway.

[John Hann]

On Tue, Apr 1, 2014 at 11:38 PM, Bill Keese <wke...@gmail.com> wrote:

I guess that means that dojo has never worked with the curl loader.  

Actually, we've got lots of folks using dojo (1.6-1.9) and curl.js together successfully.  Some rather large projects, too.

 

John, any ideas on how to get a reference to the global scope (or whatever its called)?  James thought that the  (function(){ return this; })() approach wouldn't work when "use strict" was set.   Judging from http://stackoverflow.com/questions/5447771/node-js-global-variables perhaps we should get it as typeof global === "undefined" ? window : global.

Yah, the grab-the-context-of-an-unbound-function trick hasn't been an option for a while, ever since "use strict" became popular.

Whenever I need to capture the global in a module (which is never -- unless I need to grab `window` or `document` to do DOM-ish things), I use an IIFE, like this:

(function (global) {

define(function () {

    // use global here

});

}(typeof window !== 'undefined' ? window : null));

If you need to do something global in several environments, you can sniff for plenty more (as you suggested).  This code also sniffs for the global in web sockets:

typeof window !== 'undefined' ? window : typeof global !== 'undefined' ? global : typeof self !== 'undefined' ? self : null;

 

Some may argue that AMD modules shouldn't try to set/get global variables, but OTOH it seems OK for applications to use global variables (ex: <div onclick="myOnclickMethod();">), and AMD modules may need to access those globals.  That's the case for dojo's parser anyway.

There's never a use case wherein DOM0 events bound to global methods is the best solution.  The code you've shown,  <div onclick="myOnclickMethod();">, is global and sync by nature.  Globals create hard-to-find implicit dependencies.  Sync code limits potential solutions and often leads to performance bottlenecks.

In fact, the ES6 folks haven't quite said it explicitly, but it's clear that this type of code is inconsistent and [in many cases] incompatible with modules.  They actually declared it something like this (paraphrasing): "globals and scripts are synchronous by nature and modules are async".  

The dojo/on module allows for more -- and in all but the most trivial cases, simpler -- async solutions.  Imho, dojo should be strongly coercing folks away from globals as well as DOM0 for real applications.

Just my 2¢ :)

[Bill Keese]

On Wednesday, April 2, 2014 10:01:27 PM UTC+9, unscriptable wrote:

Actually, we've got lots of folks using dojo (1.6-1.9) and curl.js together successfully.  Some rather large projects, too.

Yes, but what I meant was that dojo.global is presumably set incorrectly, and that will break various things, like when dojo/parser tries to look up globals, ex: <div data-dojo-type=dijit/tree store=myGlobal>.  I didn't mean that every single dojo feature was broken.

 

Whenever I need to capture the global in a module (which is never -- unless I need to grab `window` or `document` to do DOM-ish things), I use an IIFE, like this:

(function (global) {

define(function () {

    // use global here

});

}(typeof window !== 'undefined' ? window : null));

OK, that's similar to what James suggested.  Why use an IIFE rather than just something like this?

define([....], function(){

   var global = typeof window !== 'undefined' ? window : null

  ...

});

  

 

There's never a use case wherein DOM0 events bound to global methods is the best solution. 

I was just using events as an illustration.  The real use case we have is the one I listed at the beginning of this message:  <div data-dojo-type=dijit/tree store=myGlobal>, i.e. when the app has specified the name of a global variable to us, and we need to look it up.   You'll probably argue that we should desupport/discourage that capability too, but that's sort of a separate discussion.

[John Hann]

On Wed, Apr 2, 2014 at 10:00 AM, Bill Keese <wke...@gmail.com> wrote:

On Wednesday, April 2, 2014 10:01:27 PM UTC+9, unscriptable wrote:

Actually, we've got lots of folks using dojo (1.6-1.9) and curl.js together successfully.  Some rather large projects, too.

Yes, but what I meant was that dojo.global is presumably set incorrectly, and that will break various things, like when dojo/parser tries to look up globals, ex: <div data-dojo-type=dijit/tree store=myGlobal>.  I didn't mean that every single dojo feature was broken.

I just peeked and found that dojo.global is set in dojo/_base/kernel.  This is not an AMD-wrapped CommonJS module, so it works fine: https://github.com/dojo/dojo/blob/master/_base/kernel.js#L21

 

 

Why use an IIFE rather than just something like this?

define([....], function(){

   var global = typeof window !== 'undefined' ? window : null

  ...

});

Just to get the ugly, non-essential code out of the way. :)  I also feel better doing global things outside of the module factory, but that's just me being silly, I guess.  It seems I have convinced myself that globals don't exist inside modules.  This is not true, of course, but it feels like the right way to think about modules imho.

You'll probably argue that we should desupport/discourage that capability too, but that's sort of a separate discussion.

Yes, I would, and, yes, you are correct. :)  

[Bill Keese]

On Wednesday, April 2, 2014 11:19:47 PM UTC+9, unscriptable wrote:

I just peeked and found that dojo.global is set in dojo/_base/kernel.  This is not an AMD-wrapped CommonJS module, so it works fine: https://github.com/dojo/dojo/blob/master/_base/kernel.js#L21

I'm confused, according to your earlier comment this is undefined in the non CommonJS case:

For maximum compat with node modules, curl.js sets the context to module.exports when the module is AMD-wrapped CommonJS.  Otherwise, it's undefined. 

 

(emphasis added).  So, I expected dojo.global to be undefined, rather than its usual value (window on browsers).   Perhaps you meant "undefined" in the sense of "we don't guarantee anything", rather than the actual javascript undefined.

[John Hann]

Hey, Bill. I was not clear.  curl.js sets the context to `undefined` when executing the factory function.  Sorta like this:

factory.apply(undefined, deps);

Since curl.js itself is not running in strict mode, the context inside the factory is `window` in browsers. 

Sorry for not being clear.

-- John

--

[James Burke]

Aggregating a few pieces of follow up on this thread:

1) requirejs always sets the `this` value to the exports object
because the r.js optimizer's default action is to inline a dependency
array to avoid function CJS toString scanning after a build. So there
is no reliable method for it to know if a module was originally a
CommonJS module that is now wrapped.

For that reason, if we were to specify what `this` should be in the
AMD spec, then I think it should just always be the exports object. If
John and another AMD implementer like Rawld or Jakob agree, then I can
work up a pull request change to the AMD doc.

2) I am open to exposing `module.global` if modules needed access to
whatever the global is for the environment. This seems analogous to
the loader.global property in ES6 loaders. But also:

3) I agree with John that modules should avoid accessing a general
global object, and best to environment detect for specific globals or
global properties that are needed, like looking for `document` (or
asking for a 'document' module that does the detection or sets up a
shim) if needing something like `document.querySelector` in the
module.

4) requirejs has set the exports value to "this" for quite a while,
even back to the 1.0 series[1], although the 1.0 series had caveats to
it, so may not always have been observable. The commit Bill mentioned
was for fixing a specific issue around access to `module.exports`
inside the factory function.

James

[1] https://github.com/jrburke/requirejs/blob/1.0.8/require.js#L1762

[John Hann]

On Wed, Apr 2, 2014 at 1:29 PM, James Burke <jrb...@gmail.com> wrote:

For that reason, if we were to specify what `this` should be in the
AMD spec, then I think it should just always be the exports object. If
John and another AMD implementer like Rawld or Jakob agree, then I can
work up a pull request change to the AMD doc.

+1 for consistency, but I would have picked `undefined` :)

I will defer to the consensus.  

 

2) I am open to exposing `module.global` if modules needed access to
whatever the global is for the environment. This seems analogous to
the loader.global property in ES6 loaders. But also:

This sounds reasonable to me, as well.

 

3) I agree with John that modules should avoid accessing a general
global object, and best to environment detect for specific globals or
global properties that are needed, like looking for `document` (or
asking for a 'document' module that does the detection or sets up a
shim) if needing something like `document.querySelector` in the
module.

I've floated the idea of a "document" module by the TC39 folks.  It makes perfect sense to me.  They don't seem to think the general public is ready to give up on globals just yet. :)  Oh well.

-- John

 

4) requirejs has set the exports value to "this" for quite a while,
even back to the 1.0 series[1], although the 1.0 series had caveats to
it, so may not always have been observable. The commit Bill mentioned
was for fixing a specific issue around access to `module.exports`
inside the factory function.

James

[1] https://github.com/jrburke/requirejs/blob/1.0.8/require.js#L1762

On Tue, Apr 1, 2014 at 1:27 PM, Bill Keese <wke...@gmail.com> wrote:
> 1. client code should get this from a closure, i.e. (function(){ return
> this; })().foo instead of just this.foo
> 2. RequireJS shouldn't mess with the value of this inside the callback; it
> should point to the global
> 3. there should be a module.global property that points to the global object