cujojs / curl

curl.js is small, fast, extensible module loader that handles AMD, CommonJS Modules/1.1, CSS, HTML/text, and legacy scripts.
https://github.com/cujojs/curl/wiki
Other
1.89k stars 216 forks source link

dojo integration. Error when localRequire replaces dojo require implementation #208

Closed greghawk closed 10 years ago

greghawk commented 10 years ago

dojo require implementation uses the has.js script to check for mobile dependencies and other features that the current browser may support. Curl.js inserts itself for require but does not have the methods "add" or "on" to name a few that dojo has amended to their require. The error experienced is that object function {}() has not method named "add". This is because in dojo has.js replaces the add method with function(){} if the require provided to the method does not have an add method. This is in dojo 1.7 and above

unscriptable commented 10 years ago

Thanks @greghawk.

We need to know how to shim these correctly. If we were to add an .add() method and an .on() method to the local require, what would they need to do to allow dojo to load/run? Any ideas?

-- John

greghawk commented 10 years ago

it looks like it would need to point to the has.js library. This was a separate project on git.

dojo/has¶ Authors: Chris Mitchell, Pete Higgins Project owner: Kris Zyp since: 1.7 dojo/has provides standardized feature detection with an extensible API. It is based on the conventions in the has.js project

greghawk commented 10 years ago

http://dojotoolkit.org/reference-guide/1.9/dojo/has.html#id4

greghawk commented 10 years ago

I actually added the code below under "createContext()" in my local curl.js just to see what it would do but at that point but it found other errors.

    function test() {}          
        function test2() {}

        localRequire['has'] = test;
        localRequire['has']['add'] = test2;
unscriptable commented 10 years ago

Hey @bryanforbes, any idea how we can figure out how to shim some of the dojo-only functions on the local require? -- John

unscriptable commented 10 years ago

Hey @greghawk, Bryan asked for a test case so we can see what dojo modules are being loaded. Is that possible? Some very simple test case? -- John

bryanforbes commented 10 years ago

I'd like to see a reduced test case showing what modules in what version of Dojo fail using curl. It's not that I don't believe that the problem exists, but I'm not sure what modules are being used to trigger the problem.

greghawk commented 10 years ago

I'll put something together. It was very basic.

On Aug 12, 2013, at 10:42 PM, Bryan Forbes notifications@github.com wrote:

I'd like to see a reduced test case showing what modules in what version of Dojo fail using curl. It's not that I don't believe that the problem exists, but I'm not sure what modules are being used to trigger the problem.

— Reply to this email directly or view it on GitHub.

greghawk commented 10 years ago

I have put together a basic example. This one hits dojo on a cdn but the error is the same regardless of local dojo or cdn. I am using wire but it should be same issue. Doesn't appear to be anything with wire as it is the localRequire that is missing some items. I added a few hacks in that function within curl but commented them out and put my name in there. How do I send it to you guys?

greghawk commented 10 years ago

I just emailed it directly to John. I am a little bit of a Git noob unfortunately. My company uses subversion (ack) and i have to manage it. When I get time I want to switch out to Git..... :)

unscriptable commented 10 years ago

I took @greghawk's code, shaved it down a bit more, and put it up on jsbin and quickly found the problem.

tl;dr: The problem is that the version of dojo/has on the CDN assumes that ppl will be loading it with the dojo loader.

dojo/has on the CDN has been built with the "dojo-has-api" has-profile property set to true. Therefore this whole block of code is missing on the CDN version of dojo/has: https://github.com/dojo/dojo/blob/master/has.js#L19-L96

The next has-block is included on the CDN version: https://github.com/dojo/dojo/blob/master/has.js#L100-L111 The first line of that assumes that the .add() method exists.

So, I guess the correct description of the problem is that dojo on the Google CDN is not compatible with foreign AMD loaders.

It seems like we need a short term and a long term solution for this. The short term solution may be to add a require.has() to curl's dojo shim. The long term may be to find a way to allow foreign loaders to use dojo on the CDN?

Thoughts @greghawk and @bryanforbes?

-- J

greghawk commented 10 years ago

I actually downloaded dojo 1.8.3 and had it local rather than cdn and tried it and had same issue. Perhaps I downloaded a version that had that same api setting? I had wondered if it was due to be local or not as I had seen all of yours and Brian's examples were loading local. Once it didn't work I thought I give you guys a shout.

I downloaded my local copy from http://download.dojotoolkit.org/release-1.8.3/

I just searched it and in dojo-1.8.3\dojo_base\configNode.js(27) and dojo-1.8.3\dojo_base\configRhino.js(71): have this setting "dojo-has-api":1

unscriptable commented 10 years ago

Ah ok. I just downloaded the zip files. It looks like the dojo-release-1.8.3.zip file has the same problem since it has had its has(string) calls reduced to has(bool). The dojo-release-1.8.3-src.zip does not have this problem.

I can see why the dojo devs want to omit the possible redefining of has(), so it makes sense that they'd make it a has-block. Let's see if @bryanforbes has any suggestions.

Maybe the answer will be to use the -src version with foreign loaders, but then there may be some other unnecessary code that gets loaded.

greghawk commented 10 years ago

I changed that setting in my local copy and it now gives an error that localRequire does not have an "on" method. this happens in the ready.js in dojo it seems. Wonder if this is like jquery "on" that dojo is emulating? Here is the stack.

TypeError: Object function localRequire(ids, callback, errback) { var cb, rvid, childDef, earlyExport;

            // this is public, so send pure function
            // also fixes issue #41
            cb = callback && function () { callback.apply(undef, arguments[0]); };

            // RValue require (CommonJS)
            if (isType(ids, 'String')) {
                if (cb) {
                    throw new Error('require(id, callback) not allowed');
                }
                // return resource
                rvid = toAbsId(ids, true);
                childDef = cache[rvid];
                if (!(rvid in cache)) {
                    // this should only happen when devs attempt their own
                    // manual wrapping of cjs modules or get confused with
                    // the callback syntax:
                    throw new Error('Module not resolved: '  + rvid);
                }
                earlyExport = isPromise(childDef) && childDef.exports;
                return earlyExport || childDef;
            }
            else {
                when(core.getDeps(core.createContext(cfg, def.id, ids, isPreload)), cb, errback);
            }
        } has no method 'on'
at http://localhost/requestindy.web/js/lib/dojo-1.8.3/dojo/ready.js:31:4
at Object.core.executeDefFunc (http://localhost/requestindy.web/js/lib/curl-0.7.5/src/curl.js:834:23)
at http://localhost/requestindy.web/js/lib/curl-0.7.5/src/curl.js:393:18
at http://localhost/requestindy.web/js/lib/curl-0.7.5/src/curl.js:238:50
at core.createResourceDef.def.resolve (http://localhost/requestindy.web/js/lib/curl-0.7.5/src/curl.js:404:55)
at then.then.then (http://localhost/requestindy.web/js/lib/curl-0.7.5/src/curl.js:1233:29)
at then (http://localhost/requestindy.web/js/lib/curl-0.7.5/src/curl.js:175:49)
at Promise.then (http://localhost/requestindy.web/js/lib/curl-0.7.5/src/curl.js:189:4)
at when (http://localhost/requestindy.web/js/lib/curl-0.7.5/src/curl.js:214:26)
at CurlApi.then.then.then [as then] (http://localhost/requestindy.web/js/lib/curl-0.7.5/src/curl.js:1230:4)
at when (http://localhost/requestindy.web/js/lib/curl-0.7.5/src/curl.js:214:26) 
unscriptable commented 10 years ago

It looks like that was fixed in 1.9.0. https://github.com/dojo/dojo/blob/1.9.0/ready.js#L64

Is 1.9.0 an option for you, @greghawk?

greghawk commented 10 years ago

so that i understand this right. If i get a local copy of dojo 1.9.0 and turn off the has api setting it should work?

unscriptable commented 10 years ago

Seems like that would fix the two problems we've found so far. :)

greghawk commented 10 years ago

I'm trying it out now. My problem though is that I am using a third party library that uses dojo 1.8.3. They have extended dijit and rely on that version. I could scope them out by changing the the djconfig scope variable but then i couldn't load via curl and its dependency reader.

greghawk commented 10 years ago

alright. I downloaded 1.9.0 release to local. Changed the config value in configNode.js and configRhino.js to 0 and now i get the original "no add() method found.

unscriptable commented 10 years ago

Ok, care to try out this updated shim? It's totally untested :) , but looks right to my eye:


/** MIT License (c) copyright 2010-2013 B Cavalier & J Hann */

/**
 * curl dojo 1.8 and 1.9 shim
 *
 * Licensed under the MIT License at:
 *      http://www.opensource.org/licenses/mit-license.php
 */

/**
 * This shim overcomes some issues with dojo 1.8 and 1.9 local require().
 *
 * usage:
 * curl.config({
 *     // <packages, etc. go here>
 *
 *     // load this shim as a preload
 *     preloads: ['curl/shim/dojo18']
 * });
 */
//var require;
(function (global, doc){
define(/*=='curl/shim/dojo18',==*/ ['curl/_privileged'], function (priv) {
"use strict";

    var _curl = priv['_curl'],
        origCreateContext = priv['core'].createContext;

    var cache = {},
        element = doc && doc.createElement('div');

    var has = function (name) {
        typeof cache[name] == 'function'
            ? (cache[name] = cache[name](global, doc, element))
            : cache[name];
    };

    has.add = function (name, test, now, force) {
        if (cache[name] === undefined || force) {
            cache[name] = test;
        }
        if (now) {
            return has(name);
        }
    };

    function duckPunchRequire (req) {
        // create a functioning has()
        if (!req['has']) {
            req['has'] = has;
        }
        // create a stub for on()
        if (!req['on']) {
            req['on'] = function () {};
        }
//      // dojo 1.7 has a few unchecked `require.cache` usages
//      if (!req['cache']) req['cache'] = {};
        return req;
    }

    // modify global curl cuz dojo doesn't always use local `require`
    // as a dependency
    duckPunchRequire(_curl);

//  // dojo 1.7 still expects a global `require`, so make sure they've got one
//  if (typeof require == 'undefined') {
//      require = _curl;
//  }

    // override createContext to override "local require"
    priv['core'].createContext = function () {
        var def = origCreateContext.apply(this, arguments);
        duckPunchRequire(def.require);
        return def;
    };

    return true;

});
}(
    typeof global == 'object' ? global : this.window || this.global,
    typeof document == 'object' && document
));
greghawk commented 10 years ago

further along but now I am seeing this.

Uncaught ReferenceError: require is not defined browser.js:8 an error happened during loading packageLoader.js:31 define() missing or duplicated: js/lib/dojo-1.9.0/dojo/_base/browser.js packageLoader.js:32 Error: define() missing or duplicated: js/lib/dojo-1.9.0/dojo/_base/browser.js at http://localhost:3560/js/lib/curl-0.7.5/src/curl.js:976:19 at HTMLScriptElement.process (http://localhost:3560/js/lib/curl-0.7.5/src/curl.js:724:6) packageLoader.js:33

unscriptable commented 10 years ago

Nice work, dojo! They're assuming a global require rather than using the local require. Just uncomment the code near this string "global require" at the bottom of the shim I sent.

unscriptable commented 10 years ago

Trying the shim out myself on jsbin...

greghawk commented 10 years ago

had to uncomment the require var at top too. getting this error now.

Object function localRequire(ids, callback, errback) { var cb, rvid, childDef, earlyExport;

            // this is public, so send pure function
            // also fixes issue #41
            cb = callback && function () { callback.apply(undef, arguments[0]); };

            // RValue require (CommonJS)
            if (isType(ids, 'String')) {
                if (cb) {
                    throw new Error('require(id, callback) not allowed');
                }
                // return resource
                rvid = toAbsId(ids, true);
                childDef = cache[rvid];
                if (!(rvid in cache)) {
                    // this should only happen when devs attempt their own
                    // manual wrapping of cjs modules or get confused with
                    // the callback syntax:
                    throw new Error('Module not resolved: '  + rvid);
                }
                earlyExport = isPromise(childDef) && childDef.exports;
                return earlyExport || childDef;
            }
            else {
                when(core.getDeps(core.createContext(cfg, def.id, ids, isPreload)), cb, errback);
            }
        } has no method 'initSyncLoader' 
unscriptable commented 10 years ago

I can't seem to get that error with a very close facsimile of what you sent. However, I did find others and fixed them via shims. Here's the result: https://github.com/cujojs/curl/blob/dev/src/curl/shim/dojo18.js

Please try that when you get a chance.

"initSyncLoader" is a sign that something is triggering legacy loading behavior. Could that be the third-party code you mentioned?

greghawk commented 10 years ago

I'll give it a shot. I am leaving the third party stuff out for now. Do you think wire may be doing that?

From: John Hann notifications@github.com Reply-To: cujojs/curl <reply+i-17975990-9410c648a9df9e6a896e6948141182bd8e6016e9-5090707@reply.git hub.com> Date: Tuesday, August 13, 2013 4:27 PM To: cujojs/curl curl@noreply.github.com Cc: Greg Hawk greghawk@carolina.rr.com Subject: Re: [curl] dojo integration. Error when localRequire replaces dojo require implementation (#208)

I can't seem to get that error with a very close facsimile of what you sent. However, I did find others and fixed them via shims. Here's the result: https://github.com/cujojs/curl/blob/dev/src/curl/shim/dojo18.js

Please try that when you get a chance.

"initSyncLoader" is a sign that something is triggering legacy loading behavior. Could that be the third-party code you mentioned?

‹ Reply to this email directly or view it on GitHub https://github.com/cujojs/curl/issues/208#issuecomment-22594798 .

greghawk commented 10 years ago

I think it might be wire then. I just added your updated shim and same initSynchLoader issue

unscriptable commented 10 years ago

I gave up on my jsbin. Too many timing issues when trying to inline the modules and the shims.

Hmmm.. I am using wire in my local copy of your demo code. Have you expand your wire spec beyond just the 'wire/dojo/dijit' plugin and 'testamd' module?

greghawk commented 10 years ago

no. I am trying to keep it simple to make sure the basics work. i am testing with the exact sample i sent you other than switching to a local version of 1.9.0 as you had suggested earlier.

unscriptable commented 10 years ago

Interesting. I am testing on 1.8.3. :) I'll download and test with 1.9.

unscriptable commented 10 years ago

1.9.1 worked for me. I am using the releases in the ...-src.zip files. Are you using different files/releases?

greghawk commented 10 years ago

I am using the release version. Not source. Should that matter?

On Aug 13, 2013, at 6:33 PM, John Hann notifications@github.com wrote:

1.9.1 worked for me. I am using the releases in the ...-src.zip files. Are you using different files/releases?

— Reply to this email directly or view it on GitHub.

greghawk commented 10 years ago

I was using 1.9.0 not .1

On Aug 13, 2013, at 6:33 PM, John Hann notifications@github.com wrote:

1.9.1 worked for me. I am using the releases in the ...-src.zip files. Are you using different files/releases?

— Reply to this email directly or view it on GitHub.

unscriptable commented 10 years ago

I tried 1.9.0 and 1.9.1. I tried the -src.zip and the other zip. The minimal app works for me with all if them when using the latest dojo18 shim.

Hm... I had a problem with git and/or GitHub earlier today. can you check that you're using the latest shim in curl's Dev branch?

greghawk commented 10 years ago

we definitely have something different then. In that loader config meld is not listed. Curl errors looking for js/meld.js which equates to [basepath]/<curl doesn't know where else to look>.

So the question is: How is your version of wire not looking for meld.js? These are the versions I have (all local):

dojo-1.9.0 curl-0.7.5 when-2.2.1 wire-0.10.1

(just added to test project) meld-1.3.0

unscriptable commented 10 years ago

I added it to the project, and am seeing the error you see. I found a bug in my dojo18 shim and fixed it, but am seeing more issues. I'll continue to root these out until this simple project works. :)

unscriptable commented 10 years ago

Ok. I've fixed a few more things. The dojo code still makes a lot of assumptions and the production builds definitely assume that the dev will use the dojo loader. Stil, I think I got it this time. corsses fingers The project loads correctly in my sandbox now. \o/

Latest shim is here: https://github.com/cujojs/curl/blob/dev/src/curl/shim/dojo18.js

I imagine we might have an issue or two when you start working with dijit. If you do, please send me the updates to your test project and I'll help figure out what's happening again.

greghawk commented 10 years ago

hmm. That file says updated 19 hours ago?

greghawk commented 10 years ago

I really appreciate your work on this! Have you had a lot of interest in IoC, the AMD loader and such with cujo?

unscriptable commented 10 years ago

Git is acting very strange on my machine today. I just added support for passing in a has-profile from the config and pushed again. This time it looks like it worked: https://github.com/cujojs/curl/blob/dev/src/curl/shim/dojo18.js

We've had a ton of interest in AMD lately, but a bit less for IOC. I don't think many js devs are schooled in IOC. That said, the folks who do have an IOC background (or any inkling of inverted thinking) go absolutely crazy for wire. :)

I appreciate your help getting the dojo shim working. I suspect there are a higher percentage of dojo folks who know IOC, than the general web dev population. :)

greghawk commented 10 years ago

its working! I even got placeAt in wire to show the div with some simple text on it to the page...

unscriptable commented 10 years ago

Cool! I'm going to close this issue since the basic support seems to be in place. Please open other tickets if you find bugs or problems. Also: we're on freenode in the #cujojs group if you want to chat live. There are a number of dojo folks there, too.