-
Notifications
You must be signed in to change notification settings - Fork 84
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
@@ -38,7 +38,8 @@ up.Request.XHRRenderer = class XHRRenderer { | |||
} | |||
|
|||
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.
@@ -38,7 +38,8 @@ up.Request.XHRRenderer = class XHRRenderer { | |||
} | |||
|
|||
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
act
and the tests still failed. -
The
buildAndSend
function is notasync
, so anasync/await
implementation wouldn't work, which is why I chosePromise.resolve()
, which reliably flattens and/or wraps any result fromthis._getPayload()
into aPromise
You are correct that passingxhr.send
as a value loses thexhr
as the currentthis
whenxhr.send
is 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 thatxhr
is captured as the currentthis
for 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