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

IE10 resolves before scripts are executed #131

Closed unscriptable closed 11 years ago

unscriptable commented 11 years ago

Reported by @valueof. see comment thread here: http://twitter.com/valueof/status/243153187056545792

According to @davemethvin, IE10 hasn't run the script until after the script elements readyState == 'complete'. This is a deviation from all earlier versions of IE that run the script when readyState == 'interactive'. This is the only state at which we can associate the script request with the define() in IE 6-9. Unfortunately, IE10 still fires the readystatechange event for the 'interactive' state, so this is a problem.

Options:

  1. sniff for IE10 (ugh) like we do with Opera.
  2. associate a script request with a define() and just remember the association, but still wait for the 'complete' state before proceeding.

Option 2 seems like the best choice even though it will slow down IE6-9 very slightly. This may let us remove the Opera sniff, too.

asilvas commented 11 years ago

Would option 2 even work if there were multiple defines within a given module? Sounds like it'd be ok since it still must wait for the complete state.

unscriptable commented 11 years ago

tl;dr: Yes, it's ok to wait for "complete". :)

Sorry, to be clear: only anonymous modules/define()s are associated with requested scripts in this manner. This mechanism (watching for the "interactive" state) is how anonymous modules get their names in IE6-9. :) The only way to have multiple modules in a file (such as in optimized files) is to include the module ids (aka "named modules"). Named modules already have their names, so they don't use this mechanism.

Other browsers -- and seemingly the latest version of IE10 -- run the script element's load event immediately after the script executes, so it's easy to associate.

asilvas commented 11 years ago

Makes sense.

unscriptable commented 11 years ago

A fix for this is in the ie10 branch (curl.js file changed only). Feel free to try it out!

For those interested: IE10 is messed up. It still fires readystatechange events, but fires them too early. Unfortunately, we still have to use onreadystatechange for IE9, but IE10 does fine with just onload. curl.js now uses a sniff for addEventListener to stop testing for script load using el.readyState. This seems safe for future IEs.

-- John

cc @bentlegen

getify commented 11 years ago

I might steal this approach to patch into LABjs, but FWIW, LABjs uses a diff technique for all IE's (aka "true preloading"), and for whatever reason (I have a hunch) it seems (according to @dmethvin) to be immune to this problem. I don't have IE10, so I haven't confirmed one way or the other.

unscriptable commented 11 years ago

Hey @getify!

Don't steal it just yet. I'm seeing some problems in Chrome now. Probably just late-night sloppiness. I'll post the ultimate solution here.

-- J

getify commented 11 years ago

The "true preloading" approach LABjs uses in IE works like this:

  1. create a script element, point it at the source... but DO NOT add it to the DOM.
  2. listen for the "ready" state (meaning it's finished preloading, or it's already in the cache)
  3. THEN add it to the DOM (to signal to execute it).
  4. wait again for the "ready" or "complete" states (should always be "complete" in this case, I think), which lets you know it was finished executing.

I think because we do the async task of inserting into the DOM after we hear the "ready" state, that's long enough, delayed enough, that the script really does execute, before the library continues on and lets other parts know it's ready to go.

unscriptable commented 11 years ago

I really, really like your "true preloading" mechanism. It's the awesomest.

However, it seems MS is willing to break legacy behavior in order to get IE9 and IE10 to conform to standards. As a result, this is the second time they've broken script loaders by changing the timing of load/readystatechange events versus the evaluation of the script since IE8. (FWIW, curl.js wasn't affected by the change in IE9, but RequireJS and others were.)

As a result, I'm leaning towards code that works uniformly in all browsers at the moment.

getify commented 11 years ago

the "true preloading" behavior, as described, IS actually in the standard... as a "may" suggestion, not a requirement, but in the spec nonetheless.

but yes, this is another colossal fuck up by microsoft, and certainly not the last.

unscriptable commented 11 years ago

lazy: do you have a link to the "may" suggestion?

getify commented 11 years ago

it keeps changing locations as the spec changes (hard to get a permalink to it), but it can currently be found here:

http://www.w3.org/TR/html5/the-script-element.html#script-processing-src-prepare

Last paragraph of step 14, starting with "For performance reasons..."

unscriptable commented 11 years ago

this fix is in the dev branch now

asilvas commented 11 years ago

Is this bug applicable to the "js!" plugin? Also, it was not clear to me if this bug is only visible to IE10 users for anonymous modules only or if it is also applicable to named modules (define("mod1")) as well. Is there a unit test by chance? Trying to better understand the impact of this issue, with the upcoming IE10 release.

unscriptable commented 11 years ago

Yes. it affects the js! plugin, too. It affects both anonymous and named modules since IE10 is firing events before the script is evaluated (so curl doesn't even know if there are named modules or not, yet).

The following functional test exhibits the behavior, but note the failure can be intermittent so Cmd-R or F5 a few times. :)

https://github.com/cujojs/curl/blob/062a/test/plainOldJs.html

asilvas commented 11 years ago

Is the fix only in the loader (curl.js?), as I noticed no changes to js.js in the plugins (branch 062a).

unscriptable commented 11 years ago

Strange. You're not seeing this? https://github.com/cujojs/curl/commit/a9c4cc47252e4a4d2340ca75133ae7cb7eb79c77#src/curl/plugin/js.js

-- J

On Tue, Oct 9, 2012 at 3:14 PM, Aaron notifications@github.com wrote:

Is the fix only in the loader (curl.js?), as I noticed no changes to js.js in the plugins (branch 062a).

— Reply to this email directly or view it on GitHubhttps://github.com/cujojs/curl/issues/131#issuecomment-9275153.

asilvas commented 11 years ago

My mistake, what threw me off is the js plugin is exactly the same as the one in 0.6.8. Thanks.