Skip to content

Conversation

robertream
Copy link

@robertream robertream commented May 12, 2025

#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

@@ -38,7 +38,8 @@ up.Request.XHRRenderer = class XHRRenderer {
}

Object.assign(xhr, handlers)
xhr.send(this._getPayload())

Promise.resolve(this._getPayload()).then(xhr.send)
Copy link
Contributor

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)
Copy link
Contributor

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())

Copy link
Author

@robertream robertream May 13, 2025

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

  1. 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.

  2. The buildAndSend function is not async, so an async/await implementation wouldn't work, which is why I chose Promise.resolve(), which reliably flattens and/or wraps any result from this._getPayload() into a Promise You are correct that passing xhr.send as a value loses the xhr as the current this when xhr.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 called xhr.send(payload) directly, so thatxhr is captured as the current this for the .send() function.

Copy link
Contributor

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.

@robertream robertream force-pushed the rr/promise-payload branch from 3a60442 to 59ca2e0 Compare May 13, 2025 21:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants