Track XHR replacements correctly (#2455)

* Track XHR replacements correctly

Fixes #2439

* Update docs [skip ci]

docs/change-log.md

@@ -115,6 +115,7 @@
 - render: correct `contenteditable` check to also check for `contentEditable` property name ([#2450](https://github.com/MithrilJS/mithril.js/pull/2450) [@isiahmeadows](https://github.com/isiahmeadows))
 - docs: clarify valid key usage ([#2452](https://github.com/MithrilJS/mithril.js/pull/2452) [@isiahmeadows](https://github.com/isiahmeadows))
 - route: don't pollute globals ([#2453](https://github.com/MithrilJS/mithril.js/pull/2453) [@isiahmeadows](https://github.com/isiahmeadows))
+- request: track xhr replacements correctly ([#2455](https://github.com/MithrilJS/mithril.js/pull/2455) [@isiahmeadows](https://github.com/isiahmeadows))
 
 ---
 

docs/index.md

@@ -18,7 +18,7 @@ It's small (< 10kb gzip), fast and provides routing and XHR utilities out of the
 <div style="display:flex;margin:0 0 30px;">
 	<div style="width:50%;">
 		<h5>Download size</h5>
-		<small>Mithril (9.5kb)</small>
+		<small>Mithril (9.6kb)</small>
 		<div style="animation:grow 0.08s;background:#1e5799;height:3px;margin:0 10px 10px 0;transform-origin:0;width:4%;"></div>
 		<small style="color:#aaa;">Vue + Vue-Router + Vuex + fetch (40kb)</small>
 		<div style="animation:grow 0.4s;background:#1e5799;height:3px;margin:0 10px 10px 0;transform-origin:0;width:20%"></div>

docs/request.md

@@ -54,7 +54,7 @@ Argument                  | Type                              | Required | Descr
 `options.withCredentials` | `Boolean`                         | No       | Whether to send cookies to 3rd party domains. Defaults to `false`
 `options.timeout`         | `Number`                          | No       | The amount of milliseconds a request can take before automatically being [terminated](https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest/timeout).  Defaults to `undefined`.
 `options.responseType`    | `String`                          | No       | The expected [type](https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest/responseType) of the response. Defaults to `""` if `extract` is defined, `"json"` if missing. If `responseType: "json"`, it internally performs `JSON.parse(responseText)`.
-`options.config`          | `xhr = Function(xhr)`             | No       | Exposes the underlying XMLHttpRequest object for low-level configuration. Defaults to the [identity function](https://en.wikipedia.org/wiki/Identity_function).
+`options.config`          | `xhr = Function(xhr)`             | No       | Exposes the underlying XMLHttpRequest object for low-level configuration and optional replacement (by returning a new XHR).
 `options.headers`         | `Object`                          | No       | Headers to append to the request before sending it (applied right before `options.config`).
 `options.type`            | `any = Function(any)`             | No       | A constructor to be applied to each object in the response. Defaults to the [identity function](https://en.wikipedia.org/wiki/Identity_function).
 `options.serialize`       | `string = Function(any)`          | No       | A serialization method to be applied to `body`. Defaults to `JSON.stringify`, or if `options.body` is an instance of [`FormData`](https://developer.mozilla.org/en/docs/Web/API/FormData), defaults to the [identity function](https://en.wikipedia.org/wiki/Identity_function) (i.e. `function(value) {return value}`).

request/request.js

@@ -79,13 +79,13 @@ module.exports = function($window, Promise) {
 			var assumeJSON = (args.serialize == null || args.serialize === JSON.serialize) && !(body instanceof $window.FormData)
 			var responseType = args.responseType || (typeof args.extract === "function" ? "" : "json")
 
-			var xhr = new $window.XMLHttpRequest(),
+			var xhr = new $window.XMLHttpRequest(), aborted = false
-				aborted = false,
+			var original = xhr, replacedAbort
-				_abort = xhr.abort
+			var abort = xhr.abort
 
-			xhr.abort = function abort() {
+			xhr.abort = function() {
 				aborted = true
-				_abort.call(xhr)
+				abort.call(this)
 			}
 
 			xhr.open(method, url, args.async !== false, typeof args.user === "string" ? args.user : undefined, typeof args.password === "string" ? args.password : undefined)
@@ -106,47 +106,45 @@ module.exports = function($window, Promise) {
 				}
 			}
 
-			if (typeof args.config === "function") xhr = args.config(xhr, args) || xhr
+			xhr.onreadystatechange = function(ev) {
-
-			xhr.onreadystatechange = function() {
 				// Don't throw errors on xhr.abort().
-				if(aborted) return
+				if (aborted) return
 
-				if (xhr.readyState === 4) {
+				if (ev.target.readyState === 4) {
 					try {
-						var success = (xhr.status >= 200 && xhr.status < 300) || xhr.status === 304 || (/^file:\/\//i).test(url)
+						var success = (ev.target.status >= 200 && ev.target.status < 300) || ev.target.status === 304 || (/^file:\/\//i).test(url)
 						// When the response type isn't "" or "text",
 						// `xhr.responseText` is the wrong thing to use.
 						// Browsers do the right thing and throw here, and we
 						// should honor that and do the right thing by
 						// preferring `xhr.response` where possible/practical.
-						var response = xhr.response, message
+						var response = ev.target.response, message
 
 						if (responseType === "json") {
 							// For IE and Edge, which don't implement
 							// `responseType: "json"`.
-							if (!xhr.responseType && typeof args.extract !== "function") response = JSON.parse(xhr.responseText)
+							if (!ev.target.responseType && typeof args.extract !== "function") response = JSON.parse(ev.target.responseText)
 						} else if (!responseType || responseType === "text") {
 							// Only use this default if it's text. If a parsed
 							// document is needed on old IE and friends (all
 							// unsupported), the user should use a custom
 							// `config` instead. They're already using this at
 							// their own risk.
-							if (response == null) response = xhr.responseText
+							if (response == null) response = ev.target.responseText
 						}
 
 						if (typeof args.extract === "function") {
-							response = args.extract(xhr, args)
+							response = args.extract(ev.target, args)
 							success = true
 						} else if (typeof args.deserialize === "function") {
 							response = args.deserialize(response)
 						}
 						if (success) resolve(response)
 						else {
-							try { message = xhr.responseText }
+							try { message = ev.target.responseText }
 							catch (e) { message = response }
 							var error = new Error(message)
-							error.code = xhr.status
+							error.code = ev.target.status
 							error.response = response
 							reject(error)
 						}
@@ -157,6 +155,19 @@ module.exports = function($window, Promise) {
 				}
 			}
 
+			if (typeof args.config === "function") {
+				xhr = args.config(xhr, args, url) || xhr
+
+				// Propagate the `abort` to any replacement XHR as well.
+				if (xhr !== original) {
+					replacedAbort = xhr.abort
+					xhr.abort = function() {
+						aborted = true
+						replacedAbort.call(this)
+					}
+				}
+			}
+
 			if (body == null) xhr.send()
 			else if (typeof args.serialize === "function") xhr.send(args.serialize(body))
 			else if (body instanceof $window.FormData) xhr.send(body)

request/tests/test-request.js

@@ -455,19 +455,15 @@ o.spec("request", function() {
 			var failed = false
 			var resolved = false
 			function handleAbort(xhr) {
-				var onreadystatechange = xhr.onreadystatechange // probably not set yet
+				var onreadystatechange = xhr.onreadystatechange
-				var testonreadystatechange = function() {
+				xhr.onreadystatechange = function() {
-					onreadystatechange.call(xhr)
+					onreadystatechange.call(xhr, {target: xhr})
 					setTimeout(function() { // allow promises to (not) resolve first
 						o(failed).equals(false)
 						o(resolved).equals(false)
 						done()
 					}, 0)
 				}
-				Object.defineProperty(xhr, "onreadystatechange", {
-					set: function(val) { onreadystatechange = val },
-					get: function() { return testonreadystatechange }
-				})
 				xhr.abort()
 			}
 			request({method: "GET", url: "/item", config: handleAbort}).catch(function() {
@@ -477,6 +473,40 @@ o.spec("request", function() {
 					resolved = true
 				})
 		})
+		o("doesn't fail on replaced abort", function(done) {
+			mock.$defineRoutes({
+				"GET /item": function() {
+					return {status: 200, responseText: JSON.stringify({a: 1})}
+				}
+			})
+
+			var failed = false
+			var resolved = false
+			var abortSpy = o.spy()
+			var replacement
+			function handleAbort(xhr) {
+				var onreadystatechange = xhr.onreadystatechange
+				xhr.onreadystatechange = function() {
+					onreadystatechange.call(xhr, {target: xhr})
+					setTimeout(function() { // allow promises to (not) resolve first
+						o(failed).equals(false)
+						o(resolved).equals(false)
+						done()
+					}, 0)
+				}
+				return replacement = {
+					send: xhr.send.bind(xhr),
+					abort: abortSpy,
+				}
+			}
+			request({method: "GET", url: "/item", config: handleAbort}).then(function() {
+				resolved = true
+			}, function() {
+				failed = true
+			})
+			replacement.abort()
+			o(abortSpy.callCount).equals(1)
+		})
 		o("doesn't fail on file:// status 0", function(done) {
 			mock.$defineRoutes({
 				"GET /item": function() {
@@ -521,18 +551,58 @@ o.spec("request", function() {
 				}
 			})
 		})
-		/*o("data maintains after interpolate", function() {
+		o("params unmodified after interpolate", function() {
 			mock.$defineRoutes({
-				"PUT /items/:x": function() {
+				"PUT /items/1": function() {
-					return {status: 200, responseText: ""}
+					return {status: 200, responseText: "[]"}
 				}
 			})
-			var data = {x: 1, y: 2}
+			var params = {x: 1, y: 2}
-			var dataCopy = Object.assign({}, data);
+			var p = request({method: "PUT", url: "/items/:x", params: params})
-			request({method: "PUT", url: "/items/:x", data})
 
-			o(data).deepEquals(dataCopy)
+			o(params).deepEquals({x: 1, y: 2})
-		})*/
+
+			return p
+		})
+		o("can return replacement from config", function() {
+			mock.$defineRoutes({
+				"GET /a": function() {
+					return {status: 200, responseText: "[]"}
+				}
+			})
+			var result
+			return request({
+				url: "/a",
+				config: function(xhr) {
+					return result = {
+						send: o.spy(xhr.send.bind(xhr)),
+					}
+				},
+			})
+				.then(function () {
+					o(result.send.callCount).equals(1)
+				})
+		})
+		o("can abort from replacement", function() {
+			mock.$defineRoutes({
+				"GET /a": function() {
+					return {status: 200, responseText: "[]"}
+				}
+			})
+			var result
+
+			request({
+				url: "/a",
+				config: function(xhr) {
+					return result = {
+						send: o.spy(xhr.send.bind(xhr)),
+						abort: o.spy(),
+					}
+				},
+			})
+
+			result.abort()
+		})
 	})
 	o.spec("failure", function() {
 		o("rejects on server error", function(done) {

test-utils/tests/test-xhrMock.js

@@ -61,6 +61,23 @@ o.spec("xhrMock", function() {
 			}
 			xhr.send("a=b")
 		})
+		o("passes event to onreadystatechange", function(done) {
+			$window.$defineRoutes({
+				"GET /item": function(request) {
+					o(request.url).equals("/item")
+					return {status: 200, responseText: "test"}
+				}
+			})
+			var xhr = new $window.XMLHttpRequest()
+			xhr.open("GET", "/item")
+			xhr.onreadystatechange = function(ev) {
+				o(ev.target).equals(xhr)
+				if (xhr.readyState === 4) {
+					done()
+				}
+			}
+			xhr.send()
+		})
 		o("handles routing error", function(done) {
 			var xhr = new $window.XMLHttpRequest()
 			xhr.open("GET", "/nonexistent")

test-utils/xhrMock.js

@@ -72,7 +72,7 @@ module.exports = function() {
 				self.readyState = 4
 				if (args.async === true) {
 					callAsync(function() {
-						if (typeof self.onreadystatechange === "function") self.onreadystatechange()
+						if (typeof self.onreadystatechange === "function") self.onreadystatechange({target: self})
 					})
 				}
 			}