-
Notifications
You must be signed in to change notification settings - Fork 85
Allow for Promise payload #742
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
| Object.assign(xhr, handlers) | ||
| xhr.send(this._getPayload()) | ||
|
|
||
| Promise.resolve(this._getPayload()).then(xhr.send) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This introduces a race condition. When this up.Request is aborted while waiting for payload processing, we must not send a request.
To address this we would need to change up.Request.prototype.load(), which currently looks like this:
new up.Request.XHRRenderer(this).buildAndSend({ ... })It probably needs to look something this:
let renderer = new up.Request.XHRRenderer(this)
// Wait for async payload processing
await renderer.build({ ... })
// Don't send the request if someone has aborted us while waiting for the renderer
if (this.state === 'loading') renderer.send()We should have a test for this.
| Object.assign(xhr, handlers) | ||
| xhr.send(this._getPayload()) | ||
|
|
||
| Promise.resolve(this._getPayload()).then(xhr.send) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The then(xhr.send) loses the xhr receiver, so send(payload) is called on an undefined this.
This might be why you see failing tests.
You must bind the xhr or do something like this:
xhs.send(await this._getPayload())There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have two thoughts here
-
I meant that the tests are failing locally for me regardless of my changes, so attempting to set up a valid test case that I had confidence in was not feasible. I even ran the GitHub actions CI workflow locally using
actand the tests still failed. -
The
buildAndSendfunction is notasync, so anasync/awaitimplementation wouldn't work, which is why I chosePromise.resolve(), which reliably flattens and/or wraps any result fromthis._getPayload()into aPromiseYou are correct that passingxhr.sendas a value loses thexhras the currentthiswhenxhr.sendis finally invoked. To addressed both of the design issues you have raised in your review, I believe that the most minimally scoped change would be to pass a closure to.then()that both checked for an aborted request and conditionally calledxhr.send(payload)directly, so thatxhris captured as the currentthisfor the.send()function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant that the tests are failing locally for me regardless of my changes, so attempting to set up a valid test case that I had confidence in was not feasible. I even ran the GitHub actions CI workflow locally using act and the tests still failed.
I don't know how to help you here. The GitHub actions run successfully with every PR, including this one.
3a60442 to
59ca2e0
Compare
#740
I would have submitted tests with this PR, but I couldn't get the test suite passing locally. Either I am doing it wrong or the tests really ought to be fixed.
Required for robertream/unpoly-json-form#12