Handle loader plugins used together with dojo/has plugin. Fixes #17438

[chuckdumont]

No description provided.

[neonstalwart]

1. how is this change related to what you're fixing?
2. can you avoid using arguments.callee?
3. can you explain how module is ever an array of arrays? if not, maybe this could be simplified to

if (module) {
    // ensure module is an array
    module = [].concat(module);
    // add modules to deps
    deps.push.apply(deps, module);
}
else {
    bc.log("amdMissingDependency", ["module", resource.mid, "dependency", dep]);
}

if module is ever an array of arrays then rather than using arguments.callee, it would be clearer to use a named function and recurse by calling it

// somewhere where deps is in scope...
function pushDeps(module) {
    if (lang.isArray(module)) {
        module.forEach(pushDeps);
    } 
    else if (module) {
        deps.push(module);
    }
    else {
        bc.log( ... );
    }
}

// later on in the try...
var module = getAmdModule(dep, resource);
pushDeps(module);

[chuckdumont]

1. The change is necessary due to the changes in the has plugin handler
2. OK. Didn't realize arguments.callee was a problem.
3. getAmdModule() in depScan.js can return either a module or an array of modules. Arrays of modules result from processing plugins. The change to the has plugin can result in recursion in plugin processing (i.e. has plugin indirectly invokes text plugin), which in turn can result in nested arrays returned by getAmdModule()

[chuckdumont]

Alternatively, it looks like the nested arrays could be avoided altogether with the following change in has.js. This is probably a better solution.

                    module = getAmdModule(resolvedId, referenceModule);
                if(module){
                    referenceModule.text = referenceModule.text.replace(regex, resolvedId);
-                   return [module];
+                   return lang.isArray(module) ? module : [module];
                }else{
                    bc.log("dojoHasMissingMid", ["plugin resource id", id, "resolved plugin resource id", moduleInfo.mid, "reference module id", referenceModule && referenceModule.mid]);
                    return getHasPluginDependency();

[neonstalwart]

i was just going to suggest a similar thing. here's what i propose

• move getAmdModule to be a method on bc - copy/paste is just a bad smell to begin with so i think we should make that right.
• change getAmdModule by replacing

return pluginProc.start(match[2], ref, bc);

with

// flatten the list of modules returned from the plugin
var modules = [];
return modules.concat.apply(modules, pluginProc.start(match[2], ref, bc));

i believe that this should flatten the list of modules returned by any plugin (ie, this fix is not specific to just the has plugin) and avoid needing to recurse in depsScan.

[chuckdumont]

OK. What's the procedure for amending a pull request? Do I just update the commit and push to the remote?

[neonstalwart]

yeah, you can either push more changes to the same branch (to add to what you have) or you can force push to the same branch (to replace what you have). either way, this PR will automatically be updated

[neonstalwart]

that looks good. thanks.

@rcgill are you happy for it to be merged?

[chuckdumont]

Also, regarding the CLA. I've been informed that as an IBM employee, I'm already covered by IBM's CCLA.

[cjolif]

I confirm @chuckdumont contributions are covered by IBM CCLA.

[neonstalwart]

i think the changes are good to be merged i'm just not sure if i'm overstepping boundaries for me to commit this myself - maybe i'm being too cautious. if someone else with commit privileges (like @cjolif) wants to take responsibility for committing it then it's not up to me to say no.

[chuckdumont]

Note that in moving the function getAmdModule to the bc, I changed the closure scope reference resource to the formal parameter reference referenceModule instead. This can be done because getAmdModule was only ever called with referenceModule === resource.

[cjolif]

I thought I answered to @neonstalwart but apparently not. So if @rcgill does not step in I think you are definitely better positioned than me to merge this as you clearly know that code better than me.

[cjolif]

ok so if nobody has objection I'm going to merge this in master and 1.9.

[chuckdumont]

Thanks Chrisophe!

[cjolif]

While doing testing before merging this is working fine on Node.JS but seems to be causing some trouble on Rhino at least in a test case. @chuckdumont have you tried it on Rhino? @clmath is also looking into find the origin of the issue which might be linked to the changes made to getAmdModule.

[clmath]

I am sorry, there was a "debugger;" statement left in my code and that's why Rhino was not working.
After removing it I confirm the code is working well in Rhino and NodeJS.

[cjolif]

ok thanks @clmath I'll run a few more tests and merge.

[cjolif]

merged in 94a0d23