-
Notifications
You must be signed in to change notification settings - Fork 21
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
Free Listings + Paid Ads: Fetch the number of syncable products for the product feed status section #1706
Free Listings + Paid Ads: Fetch the number of syncable products for the product feed status section #1706
Conversation
… products and for returning the result
… later use during the onboarding flow
* @typedef {Object} SyncableProductsCalculation | ||
* @property {number|null} count The number of syncable products. `null` if it's still in the calculation. | ||
* @property {()=>Promise} request The function requesting the start of a calculation. | ||
* @property {()=>Promise} retrieve The function retrieving the result of a requested calculation by polling with 5-second timer. |
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.
Not sure but I guess instead of by polling
you mean by pulling
?
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.
Just my 2 cents, to me polling is usually used when a request is periodically repeated (which seems to be the case here). Either one could be correct though.
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.
Since the client side has no way of knowing when the needed data will be ready, it queries the data by asking periodically. The implementation here matches the scenario, so the term polling is used.
* | ||
* If a shop has a large number of products, requesting the result with a single API may encounter | ||
* a timeout or out-of-memory problem. Therefore, we use an API to schedule a batch for calculating, | ||
* and another one to poll the result. |
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.
Same as https://github.com/woocommerce/google-listings-and-ads/pull/1706/files#r985802794
I think the verb is pull
}, [ fetch, startCountdown ] ); | ||
|
||
useEffect( () => { | ||
if ( second === 0 && callCount > 0 && count === null ) { |
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.
❓ Why we need second === 0 && callCount > 0
and the dependency of second?
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'm not sure if callCount
is each time we call startCountdown
or it is each time the counter reset (after reaching 5)
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.
Why we need
second === 0 && callCount > 0
and the dependency of second?
It's up to the hook's consumer to decide when to call retrieve
, and then retrieve
will request data and schedule a timer for the next call of retrieve
triggered by this useEffect
. But useEffect
will always be called a time once the associated component is mounted, therefore, this condition is to skip the call when mounted.
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'm not sure if
callCount
is each time we callstartCountdown
or it is each time the counter reset (after reaching 5)
* @property {number} callCount The calling count of `startCountdown` 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.
LGTM ✅
I place some comment in regards one potential typo and some curiosity question.
I tested locally with and without forcing null and it works fine :)
Changes proposed in this Pull Request:
This PR implements an item of the 📌 Client states management and API integration in #1610.
ProductFeedStatusSection
component.Screenshots:
Detailed test instructions:
null
locally.google-listings-and-ads/src/API/Site/Controllers/MerchantCenter/SyncableProductsCountController.php
Line 76 in 2afa78f
wc/gla/mc/syncable-products-count
API being issued every 5 seconds.Changelog entry