Skip to content

Ensure React useAsync calls createPromise once at mount#4013

Merged
aduth merged 2 commits intomasterfrom
aduth-use-async-once
Aug 6, 2020
Merged

Ensure React useAsync calls createPromise once at mount#4013
aduth merged 2 commits intomasterfrom
aduth-use-async-once

Conversation

@aduth
Copy link
Copy Markdown
Contributor

@aduth aduth commented Aug 5, 2020

Why: The read function returned by useAsync is expected to be called multiple times, but it is intended to operate on a single promise created once at mount time (or at least once per unique set of arguments). Prior to these changes, this was behaving wrongly by creating a new promise each time read was called. While this was not particularly troublesome for the stubbed upload behavior, in real-world testing it showed to trigger repeated API requests.

Included in these changes are improvements to the test suite to try to provide stronger guarantees that both uploading and useAsync's default behavior will not make repeated calls to create a new promise.

Implementation Notes:

This is a pretty unfortunate oversight on my part, and shows that there's a fair bit of complexity in this implementation. It raises a question as to whether the form submission should occur via some more direct means: Calling upload at the time of DocumentCapture's onComplete handling, rather than managing this as a combination of component state and lifecycle side effects. That being said, there would still be complexity in managing the more "direct" path, since it needs to handle many states (pre-submission, mid-submission, post-submission success, and post-submission failure) that would likely need to be absorbed into DocumentCapture. The intent of the Submission component and useAsync hook was to try to encapsulate this, and to isolate the responsibilities of each more appropriately.

aduth added 2 commits August 5, 2020 17:25
**Why**: Conflicts with Prettier opinionated formatting
**Why**: The `read` function returned by `useAsync` is expected to be called multiple times, but it is intended to operate on a single promise created once at mount time (or at least once per unique set of arguments). Prior to these changes, this was behaving wrongly by creating a new promise each time `read` was called. While this did not prove to be troublesome for the stubbed upload behavior, in real-world testing it showed to trigger repeated API requests.
Copy link
Copy Markdown
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@aduth aduth merged commit b328840 into master Aug 6, 2020
@aduth aduth deleted the aduth-use-async-once branch August 6, 2020 12:05
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