Stop shimming async for requirejs #206
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
It does support it, support was just slightly broken. Have forked async to fix, now using that fork. Using the global meant that it was actually using the global async that was defined most recently. Which, for browser tests, happened to be the (rather) old one that karma-nodeunit defines inline (!) (https://github.com/karma-runner/karma-nodeunit/blob/master/lib/nodeunit.js#L500)
paddybyers
added a commit
that referenced
this pull request
Jan 21, 2016
Stop shimming async for requirejs
|
Thanks |
|
Jeez, that's a shitty bug 😖 Nice one for finding and fixing it |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Most-frustrating-to-debug bug so far in IE8/9 project turns out not to have been an IE bug at all!
Was looking at a test that used async.parallel, where in IE8, async wasn't calling the result callback even though all the subtests were calling their callbacks. But
console.logstatements in the functions in async.js somehow weren't getting printed, even though the file was definitely getting loaded.Turns out, when the tests were run in the browser, the
asyncthat was getting used is not the one pointed to by named_dependencies (node_modules/async/lib/async), but rather a rather old version that's copied-and-pasted inline into karma-nodeunit (!) -- https://github.com/karma-runner/karma-nodeunit/blob/master/lib/nodeunit.js#L500.Why was it using that one? Because when setting up requirejs in the browser, we apparently put a shim which tells requirejs to use the global
asyncobject, whatever that may be -- ie whichever async was loaded most recently, which was the nodeunit one.So: this PR removes the shim, and points to a branch of async in ably-forks with the issue that was causing async's requirejs-loader not to work with ably-js fixed.