dojo shim race condition (`createContext` can run before shim loads)

kfranqueiro commented 11 years ago

Commit 39548ef4a8206e2bd04dde523f38992aa93e1e89 (after 0.6.2) changed the dojo shim from wrapping executeDefFunc to createContext. Unfortunately, this causes the shim to break because createContext may run for modules before the shim is actually finished loading.

You can try the following test case to demonstrate the issue: (assumes curl and dojo 1.7 are in the same folder as the html file)

<!DOCTYPE html>
<html>
    <body>
        <script src="curl/src/curl.js"></script>
        <script>
            curl({
                packages: [
                    {
                        name: 'curl',
                        location: 'curl/src/curl'
                    },
                    'dojo'
                ],
                preloads: ['curl/shim/dojo16']
            }, ['dojo/ready'], function (ready) {
                ready(function () {
                    document.body.appendChild(
                        document.createTextNode('Hello world!')
                    );
                });
            });
        </script>
    </body>
</html>

If you run this against 0.6.2, it will work. If you run it against 0.6.5, it will fail. (If you run it against 0.6.3 or 0.6.4, it won't find the preload - I guess something temporarily broke regarding preloads honoring packages.)

I'm not really sure what the right fix to this is - I don't understand the loader deeply enough to fathom the repercussions of simply reverting the aforementioned changeset; it sounds like there was a valid reason for applying it.

Background: I've been working on updating the dojo shim since it will have issues with dojo/ready in Dojo 1.8 (though hopefully not 1.9 and future 1.8.x if http://bugs.dojotoolkit.org/ticket/15616 resolves things adequately), but was banging my head against the wall for a good couple of hours wondering why my modifications didn't seem to be taking effect - the shim having not applied by the time dojo/ready is requested is a pretty good reason I guess. :)

unscriptable commented 11 years ago

Hey @kfranqueiro, thanks so much for looking into this. It'd be nice to have dojo compatibility again. I hear that we also need to implement a require.on() method?

I'll dig into this later to figure out the correct internal method(s) to shim. curl 0.8 will have a totally different internal structure, btw, so I'll have to update the shim again. Still, it would be great to figure out what features we need to shim before curl 0.8.

Why are you using dojo 1.7? Is it because 1.8 has more non-standard require.XXX methods/props?

-- John

kfranqueiro commented 11 years ago

I used 1.7 to report this issue because existing 1.8.x won't work at all (due to no longer relying on require.ready, and lacking require.on and require.idle), and trunk won't demonstrate it (no longer assumes existence of any of these).

Given that the next 1.8 release as well as 1.9 won't have a hard reliance on these (as of a commit made 2 nights ago), I'm debating whether the modifications I was working on will ultimately be worth adding anyway, but we should probably still update the shim for anyone stuck on 1.6 or 1.7.

unscriptable commented 11 years ago

good point about 1.6 and 1.7

unscriptable commented 11 years ago

Closing this, despite fact that dojo 1.7 might still be broken. If a user yells, we'll fix the dojo16 shim. :)

cujojs / curl

dojo shim race condition (`createContext` can run before shim loads) #191