Use effect to place order after details have been fetched. #2486
Conversation
|
|
Performance Test Results The following fails have been reported by WebpageTest. These numbers indicates a possible performance issue with the PR which requires further manual testing to validate. https://pr-2486.pwa-venia.com : LH Performance Expected 0.85 Actual 0.57, LH Best Practices Expected 1 Actual 0.92 |
|
|
||
| if (!placeOrderLoading && !hasError && orderDetailsData) { | ||
| if (orderNumber) { |
| // Fetch order details and then use an effect to actually place the | ||
| // order. If/when Apollo returns promises for invokers from useLazyQuery | ||
| // we can just await this function and then perform the rest of order | ||
| // placement. |
There was a problem hiding this comment.
Yeah, the Apollo docs say the following about the invoker:
Function that can be triggered to execute the suspended query. After being called,
useLazyQuerybehaves just likeuseQuery.
So I think they do intend for us to use called, data, and loading as returned by the hook, since they're updated by the hook itself as part of its internal lifecycle. And if we want to use the hook's return values, we have to do it as an effect, just as you've done here.
|
Other than test failures, code looks fine. |
| } | ||
|
|
||
| if (orderDetailsData && !placeOrderCalled) { | ||
| placeOrderAndCleanup(); |
There was a problem hiding this comment.
Should this not be await placeOrderAndCleanup()?
There was a problem hiding this comment.
Should this not be
await placeOrderAndCleanup()?
The function provided to useEffect isn't an async function, so we couldn't await this call.
There was a problem hiding this comment.
Nope - this is the pattern for writing effects that contain async code. You can't have a top level await because the callback passed to useEffect cannot be asynchronous itself.
|
QA Pass, good to merge after test changes and approval. |
| @@ -146,7 +147,8 @@ const getTalonProps = props => { | |||
| * beforeAll | |||
| */ | |||
|
|
|||
| beforeAll(() => { | |||
| beforeEach(() => { | |||
| jest.clearAllMocks(); | |||
There was a problem hiding this comment.
This shouldn't be necessary, since we do this at the config level.
packages/peregrine/lib/talons/CheckoutPage/__tests__/useCheckoutPage.spec.js
Outdated
Show resolved
Hide resolved
packages/peregrine/lib/talons/CheckoutPage/__tests__/useCheckoutPage.spec.js
Outdated
Show resolved
Hide resolved
packages/peregrine/lib/talons/CheckoutPage/__tests__/useCheckoutPage.spec.js
Outdated
Show resolved
Hide resolved
Description
The invoker returned as
const [getOrderDetails] = useLazyQueryis not actually a promise as we previously believed. We want to only place the order once the order details have been fetched. The code was causing a race condition because both operations were happening at the same time.Related Issue
Closes PWA-677.
Acceptance
Verification Stakeholders
Specification
Verification Steps
Screenshots / Screen Captures (if appropriate)
Checklist