-
Notifications
You must be signed in to change notification settings - Fork 120
fetchQueue._promise can be referenced when fetchQueue has become undefined #304
Comments
https://github.com/walmartlabs/thorax/blob/master/src/loading.js#L311 assigns the fetch queue. Are you saying that it's possible for the operation to complete sync under some environments? Is it possible to create a test case to cover this? It seems like this is simple enough to work around, store the return in a temp var and conditionally assign to the fetchQueue field but I worry about regression, etc. Is this something that you saw in any of our production apps? |
Yes, it seems Chrome will throw an error while attaching the script element to the DOM if the JSONP url is http and the page is loaded with https. I've seen this with our dev env when running https when the p13n content is requested (http). |
I'll work on a unit test but the issue is that the browser is causing an error to be thrown when the JSONP script element is attached to the DOM. I'll have to look at throwing an error somewhere else in the process just to simulate a synchronous error for this case. |
Fixed by #305 |
Released in 2.3.1 |
It seems that there might be a problem with 3b945df#diff-6c255b8f1d69c9f1b56a600c2a50c055
If an error occurs with the request, the error callback handler will eventually execute https://github.com/walmartlabs/thorax/blob/master/src/loading.js#L347 which will null out the fetchQueue.
This causes an issue with https://github.com/walmartlabs/thorax/blob/master/src/loading.js#L322 since the fetchQueue has just become undefined.
I assume this would only happen with a syncrhronous operation but, with a zepto ajaxJSONP call, its possible for this to error out: https://github.com/madrobby/zepto/blob/master/src/ajax.js#L117
While the code is correct (http jsonp request from https page), this masks the issue and makes it difficult to debug.
http://stackoverflow.com/questions/12216208/chrome-now-blocking-all-jsonp-requests-from-https-to-http
The text was updated successfully, but these errors were encountered: