vndb/mithril-vndb
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Make m.request work with async/await correctly. (#2428)

Browse source 
* s/xhr/request/g

`m.xhr` was a relic of the rewrite days prior to the release of v1.0.0,
before it was renamed `m.request` to align with v0.2.x. This just strips
some of that legacy naming.

* Make this work with `async`/`await` correctly.

It looked like a V8 bug, but read the two big code comments and follow
their links. It's a bit more subtle than it looks, and V8's in the right
here.
This commit is contained in:
Isiah Meadows 2019-06-10 19:48:04 -04:00 • committed by GitHub
parent 8a7eae00ed
commit 0a1a33a036
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
3 changed files with 143 additions and 59 deletions
Download patch file Download diff file Expand all files Collapse all files

1
docs/change-log.md
View file

@ -93,6 +93,7 @@
- `https://unpkg.com/mithril` is configured to receive the *minified* bundle, not the development bundle.
- The raw bundle itself remains accessible at `mithril.js`, and is *not* browser-wrapped.
- Note: this *will* increase overhead with bundlers like Webpack, Rollup, and Browserify.
- request: autoredraw support fixed for `async`/`await` in Chrome ([#2428](https://github.com/MithrilJS/mithril.js/pull/2428) [@isiahmeadows](https://github.com/isiahmeadows))
---

18
request/request.js
View file

@ -6,6 +6,16 @@ module.exports = function($window, Promise) {
var callbackCount = 0
var oncompletion
function PromiseProxy(executor) {
return new Promise(executor)
}
// In case the global Promise is some userland library's where they rely on
// `foo instanceof this.constructor`, `this.constructor.resolve(value)`, or
// similar. Let's *not* break them.
PromiseProxy.prototype = Promise.prototype
PromiseProxy.__proto__ = Promise // eslint-disable-line no-proto
function makeRequest(factory) {
return function(url, args) {
if (typeof url !== "string") { args = url; url = url.url }
@ -33,6 +43,14 @@ module.exports = function($window, Promise) {
function wrap(promise) {
var then = promise.then
// Set the constructor, so engines know to not await or resolve
// this as a native promise. At the time of writing, this is
// only necessary for V8, but their behavior is the correct
// behavior per spec. See this spec issue for more details:
// https://github.com/tc39/ecma262/issues/1577. Also, see the
// corresponding comment in `request/tests/test-request.js` for
// a bit more background on the issue at hand.
promise.constructor = PromiseProxy
promise.then = function() {
count++
var next = then.apply(promise, arguments)

183
request/tests/test-request.js
View file

@ -6,12 +6,12 @@ var xhrMock = require("../../test-utils/xhrMock")
var Request = require("../../request/request")
var Promise = require("../../promise/promise")
o.spec("xhr", function() {
var mock, xhr, complete
o.spec("request", function() {
var mock, request, complete
o.beforeEach(function() {
mock = xhrMock()
var requestService = Request(mock, Promise)
xhr = requestService.request
request = requestService.request
complete = o.spy()
requestService.setCompletionCallback(complete)
})
@ -23,7 +23,7 @@ o.spec("xhr", function() {
return {status: 200, responseText: JSON.stringify({a: 1})}
}
})
xhr({method: "GET", url: "/item"}).then(function(data) {
request({method: "GET", url: "/item"}).then(function(data) {
o(data).deepEquals({a: 1})
}).then(function() {
done()
@ -35,7 +35,7 @@ o.spec("xhr", function() {
return {status: 200, responseText: JSON.stringify({a: 1})}
}
})
xhr({url: "/item"}).then(function(data) {
request({url: "/item"}).then(function(data) {
o(data).deepEquals({a: 1})
}).then(function() {
done()
@ -47,7 +47,7 @@ o.spec("xhr", function() {
return {status: 200, responseText: JSON.stringify({a: 1})}
}
})
xhr("/item").then(function(data) {
request("/item").then(function(data) {
o(data).deepEquals({a: 1})
}).then(function() {
done()
@ -59,7 +59,7 @@ o.spec("xhr", function() {
return {status: 200, responseText: JSON.stringify({a: 1})}
}
})
xhr({method: "POST", url: "/item"}).then(function(data) {
request({method: "POST", url: "/item"}).then(function(data) {
o(data).deepEquals({a: 1})
}).then(done)
})
@ -69,7 +69,7 @@ o.spec("xhr", function() {
return {status: 200, responseText: JSON.stringify({a: 1})}
}
})
xhr("/item", {method: "POST"}).then(function(data) {
request("/item", {method: "POST"}).then(function(data) {
o(data).deepEquals({a: 1})
}).then(done)
})
@ -79,7 +79,7 @@ o.spec("xhr", function() {
return {status: 200, responseText: JSON.stringify({a: request.query})}
}
})
xhr({method: "GET", url: "/item", params: {x: "y"}}).then(function(data) {
request({method: "GET", url: "/item", params: {x: "y"}}).then(function(data) {
o(data).deepEquals({a: "?x=y"})
}).then(done)
})
@ -89,7 +89,7 @@ o.spec("xhr", function() {
return {status: 200, responseText: JSON.stringify({a: JSON.parse(request.body)})}
}
})
xhr({method: "POST", url: "/item", body: {x: "y"}}).then(function(data) {
request({method: "POST", url: "/item", body: {x: "y"}}).then(function(data) {
o(data).deepEquals({a: {x: "y"}})
}).then(done)
})
@ -99,7 +99,7 @@ o.spec("xhr", function() {
return {status: 200, responseText: JSON.stringify({a: request.query})}
}
})
xhr({method: "GET", url: "/item", params: {x: ":y"}}).then(function(data) {
request({method: "GET", url: "/item", params: {x: ":y"}}).then(function(data) {
o(data).deepEquals({a: "?x=%3Ay"})
}).then(done)
})
@ -109,7 +109,7 @@ o.spec("xhr", function() {
return {status: 200, responseText: JSON.stringify({a: JSON.parse(request.body)})}
}
})
xhr({method: "POST", url: "/item", body: {x: ":y"}}).then(function(data) {
request({method: "POST", url: "/item", body: {x: ":y"}}).then(function(data) {
o(data).deepEquals({a: {x: ":y"}})
}).then(done)
})
@ -119,7 +119,7 @@ o.spec("xhr", function() {
return {status: 200, responseText: JSON.stringify({a: request.url, b: request.query, c: request.body})}
}
})
xhr({method: "GET", url: "/item/:x", params: {x: "y"}}).then(function(data) {
request({method: "GET", url: "/item/:x", params: {x: "y"}}).then(function(data) {
o(data).deepEquals({a: "/item/y", b: {}, c: null})
}).then(done)
})
@ -129,7 +129,7 @@ o.spec("xhr", function() {
return {status: 200, responseText: JSON.stringify({a: request.url, b: request.query, c: request.body})}
}
})
xhr({method: "POST", url: "/item/:x", params: {x: "y"}}).then(function(data) {
request({method: "POST", url: "/item/:x", params: {x: "y"}}).then(function(data) {
o(data).deepEquals({a: "/item/y", b: {}, c: null})
}).then(done)
})
@ -139,7 +139,7 @@ o.spec("xhr", function() {
return {status: 200, responseText: JSON.stringify({a: request.url, b: request.query, c: JSON.parse(request.body)})}
}
})
xhr({method: "GET", url: "/item/:x", params: {x: "y"}, body: {a: "b"}}).then(function(data) {
request({method: "GET", url: "/item/:x", params: {x: "y"}, body: {a: "b"}}).then(function(data) {
o(data).deepEquals({a: "/item/y", b: {}, c: {a: "b"}})
}).then(done)
})
@ -149,7 +149,7 @@ o.spec("xhr", function() {
return {status: 200, responseText: JSON.stringify({a: request.url, b: request.query, c: JSON.parse(request.body)})}
}
})
xhr({method: "POST", url: "/item/:x", params: {x: "y"}, body: {a: "b"}}).then(function(data) {
request({method: "POST", url: "/item/:x", params: {x: "y"}, body: {a: "b"}}).then(function(data) {
o(data).deepEquals({a: "/item/y", b: {}, c: {a: "b"}})
}).then(done)
})
@ -159,7 +159,7 @@ o.spec("xhr", function() {
return {status: 200, responseText: JSON.stringify({a: request.url, b: request.query, c: request.body})}
}
})
xhr({method: "GET", url: "/item/:x", params: {x: "y", q: "term"}}).then(function(data) {
request({method: "GET", url: "/item/:x", params: {x: "y", q: "term"}}).then(function(data) {
o(data).deepEquals({a: "/item/y", b: "?q=term", c: null})
}).then(done)
})
@ -169,7 +169,7 @@ o.spec("xhr", function() {
return {status: 200, responseText: JSON.stringify({a: request.url, b: request.query, c: request.body})}
}
})
xhr({method: "POST", url: "/item/:x", params: {x: "y", q: "term"}}).then(function(data) {
request({method: "POST", url: "/item/:x", params: {x: "y", q: "term"}}).then(function(data) {
o(data).deepEquals({a: "/item/y", b: "?q=term", c: null})
}).then(done)
})
@ -179,7 +179,7 @@ o.spec("xhr", function() {
return {status: 200, responseText: JSON.stringify({a: request.url, b: request.query, c: JSON.parse(request.body)})}
}
})
xhr({method: "GET", url: "/item/:x", params: {x: "y", q: "term"}, body: {a: "b"}}).then(function(data) {
request({method: "GET", url: "/item/:x", params: {x: "y", q: "term"}, body: {a: "b"}}).then(function(data) {
o(data).deepEquals({a: "/item/y", b: "?q=term", c: {a: "b"}})
}).then(done)
})
@ -189,7 +189,7 @@ o.spec("xhr", function() {
return {status: 200, responseText: JSON.stringify({a: request.url, b: request.query, c: JSON.parse(request.body)})}
}
})
xhr({method: "POST", url: "/item/:x", params: {x: "y", q: "term"}, body: {a: "b"}}).then(function(data) {
request({method: "POST", url: "/item/:x", params: {x: "y", q: "term"}, body: {a: "b"}}).then(function(data) {
o(data).deepEquals({a: "/item/y", b: "?q=term", c: {a: "b"}})
}).then(done)
})
@ -199,7 +199,7 @@ o.spec("xhr", function() {
return {status: 200, responseText: JSON.stringify({a: request.url, b: JSON.parse(request.body)})}
}
})
xhr({method: "POST", url: "/items", body: [{x: "y"}]}).then(function(data) {
request({method: "POST", url: "/items", body: [{x: "y"}]}).then(function(data) {
o(data).deepEquals({a: "/items", b: [{x: "y"}]})
}).then(done)
})
@ -209,7 +209,7 @@ o.spec("xhr", function() {
return {status: 200, responseText: JSON.stringify({a: request.url})}
}
})
xhr({method: "GET", url: "/item/:x"}).then(function(data) {
request({method: "GET", url: "/item/:x"}).then(function(data) {
o(data).deepEquals({a: "/item/:x"})
}).then(done)
})
@ -219,7 +219,7 @@ o.spec("xhr", function() {
return {status: 200, responseText: JSON.stringify({a: request.url})}
}
})
xhr({method: "GET", url: "/item/:x"}).then(function(data) {
request({method: "GET", url: "/item/:x"}).then(function(data) {
o(data).deepEquals({a: "/item/:x"})
}).then(done)
})
@ -233,7 +233,7 @@ o.spec("xhr", function() {
return {status: 200, responseText: JSON.stringify([{id: 1}, {id: 2}, {id: 3}])}
}
})
xhr({method: "GET", url: "/item", type: Entity}).then(function(data) {
request({method: "GET", url: "/item", type: Entity}).then(function(data) {
o(data).deepEquals([{_id: 1}, {_id: 2}, {_id: 3}])
}).then(done)
})
@ -247,7 +247,7 @@ o.spec("xhr", function() {
return {status: 200, responseText: JSON.stringify({id: 1})}
}
})
xhr({method: "GET", url: "/item", type: Entity}).then(function(data) {
request({method: "GET", url: "/item", type: Entity}).then(function(data) {
o(data).deepEquals({_id: 1})
}).then(done)
})
@ -261,7 +261,7 @@ o.spec("xhr", function() {
return {status: 200, responseText: JSON.stringify({body: request.query})}
}
})
xhr({method: "GET", url: "/item", serialize: serialize, params: {id: 1}}).then(function(data) {
request({method: "GET", url: "/item", serialize: serialize, params: {id: 1}}).then(function(data) {
o(data.body).equals("?id=1")
}).then(done)
})
@ -275,7 +275,7 @@ o.spec("xhr", function() {
return {status: 200, responseText: JSON.stringify({body: request.body})}
}
})
xhr({method: "POST", url: "/item", serialize: serialize, body: {id: 1}}).then(function(data) {
request({method: "POST", url: "/item", serialize: serialize, body: {id: 1}}).then(function(data) {
o(data.body).equals("id=1")
}).then(done)
})
@ -289,7 +289,7 @@ o.spec("xhr", function() {
return {status: 200, responseText: JSON.stringify({test: 123})}
}
})
xhr({method: "GET", url: "/item", deserialize: deserialize}).then(function(data) {
request({method: "GET", url: "/item", deserialize: deserialize}).then(function(data) {
o(data).deepEquals({test: 123})
}).then(done)
})
@ -303,7 +303,7 @@ o.spec("xhr", function() {
return {status: 200, responseText: JSON.stringify({test: 123})}
}
})
xhr({method: "POST", url: "/item", deserialize: deserialize}).then(function(data) {
request({method: "POST", url: "/item", deserialize: deserialize}).then(function(data) {
o(data).deepEquals({test: 123})
}).then(done)
})
@ -317,7 +317,7 @@ o.spec("xhr", function() {
return {status: 200, responseText: ""}
}
})
xhr({method: "GET", url: "/item", extract: extract}).then(function(data) {
request({method: "GET", url: "/item", extract: extract}).then(function(data) {
o(data).deepEquals({test: 123})
}).then(done)
})
@ -331,7 +331,7 @@ o.spec("xhr", function() {
return {status: 200, responseText: ""}
}
})
xhr({method: "POST", url: "/item", extract: extract}).then(function(data) {
request({method: "POST", url: "/item", extract: extract}).then(function(data) {
o(data).deepEquals({test: 123})
}).then(done)
})
@ -346,7 +346,7 @@ o.spec("xhr", function() {
return {status: 200, responseText: ""}
}
})
xhr({method: "GET", url: "/item", extract: extract, deserialize: deserialize}).then(function(data) {
request({method: "GET", url: "/item", extract: extract, deserialize: deserialize}).then(function(data) {
o(data).equals(200)
}).then(function() {
o(deserialize.callCount).equals(0)
@ -358,7 +358,7 @@ o.spec("xhr", function() {
return {status: 200, responseText: ""}
}
})
xhr({method: "POST", url: "/item", config: config}).then(done)
request({method: "POST", url: "/item", config: config}).then(done)
function config(xhr) {
o(typeof xhr.setRequestHeader).equals("function")
@ -372,11 +372,11 @@ o.spec("xhr", function() {
return {status: 200, responseText: "[]"}
}
})
xhr("/item").then(function() {
return xhr("/item")
request("/item").then(function() {
return request("/item")
})
xhr("/item").then(function() {
return xhr("/item")
request("/item").then(function() {
return request("/item")
})
setTimeout(function() {
o(complete.callCount).equals(4)
@ -389,7 +389,7 @@ o.spec("xhr", function() {
return {status: 200, responseText: "[]"}
}
})
var promise = xhr("/item")
var promise = request("/item")
promise.then(function() {}).then(function() {})
promise.then(function() {}).then(function() {})
setTimeout(function() {
@ -403,7 +403,7 @@ o.spec("xhr", function() {
return {status: 200, responseText: "[]"}
}
})
xhr("/item", {background: true}).then(function() {})
request("/item", {background: true}).then(function() {})
setTimeout(function() {
o(complete.callCount).equals(0)
@ -416,7 +416,7 @@ o.spec("xhr", function() {
return {status: 200, responseText: ""}
}
})
xhr({method: "POST", url: "/item", config: config, headers: {"Custom-Header": "Value"}}).then(done)
request({method: "POST", url: "/item", config: config, headers: {"Custom-Header": "Value"}}).then(done)
function config(xhr) {
o(xhr.getRequestHeader("Custom-Header")).equals("Value")
@ -428,7 +428,7 @@ o.spec("xhr", function() {
return {status: 200, responseText: ""}
}
})
xhr({method: "POST", url: "/item", config: config, headers: {"Content-Type": "Value"}}).then(done)
request({method: "POST", url: "/item", config: config, headers: {"Content-Type": "Value"}}).then(done)
function config(xhr) {
o(xhr.getRequestHeader("Content-Type")).equals("Value")
@ -459,7 +459,7 @@ o.spec("xhr", function() {
})
xhr.abort()
}
xhr({method: "GET", url: "/item", config: handleAbort}).catch(function() {
request({method: "GET", url: "/item", config: handleAbort}).catch(function() {
failed = true
})
.then(function() {
@ -473,7 +473,7 @@ o.spec("xhr", function() {
}
})
var failed = false
xhr({method: "GET", url: "file:///item"}).catch(function() {
request({method: "GET", url: "file:///item"}).catch(function() {
failed = true
}).then(function(data) {
o(failed).equals(false)
@ -488,7 +488,7 @@ o.spec("xhr", function() {
return {status: 200, responseText: ""}
}
})
return xhr({
return request({
method: "GET", url: "/item",
timeout: 42,
config: function(xhr) {
@ -496,13 +496,13 @@ o.spec("xhr", function() {
}
})
})
o("set responseType to xhr instance", function() {
o("set responseType to request instance", function() {
mock.$defineRoutes({
"GET /item": function() {
return {status: 200, responseText: ""}
}
})
return xhr({
return request({
method: "GET", url: "/item",
responseType: "blob",
config: function(xhr) {
@ -518,7 +518,7 @@ o.spec("xhr", function() {
})
var data = {x: 1, y: 2}
var dataCopy = Object.assign({}, data);
xhr({method: "PUT", url: "/items/:x", data})
request({method: "PUT", url: "/items/:x", data})
o(data).deepEquals(dataCopy)
})*/
@ -530,7 +530,7 @@ o.spec("xhr", function() {
return {status: 500, responseText: JSON.stringify({error: "error"})}
}
})
xhr({method: "GET", url: "/item"}).catch(function(e) {
request({method: "GET", url: "/item"}).catch(function(e) {
o(e instanceof Error).equals(true)
o(e.message).equals("[object Object]")
o(e.response).deepEquals({error: "error"})
@ -543,7 +543,7 @@ o.spec("xhr", function() {
return {status: 500, responseText: JSON.stringify({message: "error", stack: "error on line 1"})}
}
})
xhr({method: "GET", url: "/item"}).catch(function(e) {
request({method: "GET", url: "/item"}).catch(function(e) {
o(e instanceof Error).equals(true)
o(e.response.message).equals("error")
o(e.response.stack).equals("error on line 1")
@ -555,7 +555,7 @@ o.spec("xhr", function() {
return {status: 500, responseText: "error"}
}
})
xhr({method: "GET", url: "/item"}).catch(function(e) {
request({method: "GET", url: "/item"}).catch(function(e) {
o(e.message).equals("null")
o(e.response).equals(null)
}).then(done)
@ -566,15 +566,15 @@ o.spec("xhr", function() {
return {status: 500, responseText: "error"}
}
})
var request = xhr({method: "GET", url: "/item"})
var promise = request({method: "GET", url: "/item"})
var then = o.spy()
var catch1 = o.spy()
var catch2 = o.spy()
var catch3 = o.spy()
request.catch(catch1)
request.then(then, catch2)
request.then(then).catch(catch3)
promise.catch(catch1)
promise.then(then, catch2)
promise.then(then).catch(catch3)
callAsync(function() {
callAsync(function() {
@ -592,7 +592,7 @@ o.spec("xhr", function() {
return {status: 0}
}
})
xhr({method: "GET", url: "/item"}).catch(function(e) {
request({method: "GET", url: "/item"}).catch(function(e) {
o(e instanceof Error).equals(true)
}).then(done)
})
@ -602,7 +602,7 @@ o.spec("xhr", function() {
return {status: 500, responseText: JSON.stringify({message: "error"})}
}
})
xhr({
request({
method: "GET", url: "/item",
extract: function(xhr) {return JSON.parse(xhr.responseText)}
}).then(function(data) {
@ -616,7 +616,7 @@ o.spec("xhr", function() {
return {status: 200, responseText: JSON.stringify({a: 1})}
}
})
xhr({
request({
method: "GET", url: "/item",
extract: function() {throw new Error("error")}
}).catch(function(e) {
@ -635,7 +635,7 @@ o.spec("xhr", function() {
return {status: 200, responseText: JSON.stringify({a: 1})}
}
mock.$defineRoutes(routes)
xhr({
request({
method: method, url: "/item",
config: function(xhr) {
var header = xhr.getRequestHeader("Content-Type")
@ -662,7 +662,7 @@ o.spec("xhr", function() {
}
}
mock.$defineRoutes(routes)
xhr({
request({
method: method, url: "/item", body: body,
config: function(xhr) {
var header = xhr.getRequestHeader("Content-Type")
@ -695,4 +695,69 @@ o.spec("xhr", function() {
checkSet("DELETE", {foo: "bar"})
checkSet("PATCH", {foo: "bar"})
})
// See: https://github.com/MithrilJS/mithril.js/issues/2426
//
// TL;DR: lots of subtlety. Make sure you read the ES spec closely before
// updating this code or the corresponding finalizer code in
// `request/request` responsible for scheduling autoredraws, or you might
// inadvertently break things.
//
// The precise behavior here is that it schedules a redraw immediately after
// the second tick *after* the promise resolves, but `await` in engines that
// have implemented the change in https://github.com/tc39/ecma262/pull/1250
// will only take one tick to get the value. Engines that haven't
// implemented that spec change would wait until the tick after the redraw
// was scheduled before it can see the new value. But this only applies when
// the engine needs to coerce the value, and this is where things get a bit
// hairy. As per spec, V8 checks the `.constructor` property of promises and
// if that `=== Promise`, it does *not* coerce it using `.then`, but instead
// just resolves it directly. This, of course, can screw with our autoredraw
// behavior, and we have to work around that. At the time of writing, no
// other browser checks for this additional constraint, and just blindly
// invokes `.then` instead, and so we end up working as anticipated. But for
// obvious reasons, it's a bad idea to rely on a spec violation for things
// to work unless the spec itself is clearly broken (in this case, it's
// not). And so we need to test for this very unusual edge case.
//
// The direct `eval` is just so I can convert early errors to runtime
// errors without having to explicitly wire up all the bindings set up in
// `o.beforeEach`. I evaluate it immediately inside a `try`/`catch` instead
// of inside the test code so any relevant syntax error can be detected
// ahead of time and the test skipped entirely. It might trigger mental
// alarms because `eval` is normally asking for problems, but this is a
// rare case where it's genuinely safe and rational.
try {
// eslint-disable-next-line no-eval
var runAsyncTest = eval(
"async () => {\n" +
" var p = request('/item')\n" +
" o(complete.callCount).equals(0)\n" +
// Note: this step does *not* invoke `.then` on the promise returned
// from `p.then(resolve, reject)`.
" await p\n" +
// The spec prior to https://github.com/tc39/ecma262/pull/1250 used
// to take 3 ticks instead of 1, so `complete` would have been
// called already and we would've been done. After it, it now takes
// 1 tick and so `complete` wouldn't have yet been called - it takes
// 2 ticks to get called. And so we have to wait for one more ticks
// for `complete` to get called.
" await null\n" +
" o(complete.callCount).equals(1)\n" +
"}"
)
o("invokes the redraw in native async/await", function () {
mock.$defineRoutes({
"GET /item": function() {
return {status: 200, responseText: "[]"}
}
})
return runAsyncTest()
})
} catch (e) {
// ignore - this is just for browsers that natively support
// `async`/`await`, like most modern browsers.
// it's just a syntax error anyways.
}
})