feat(docgen-sidebar): add polling to tags loading in dogen sidebar #3858
Conversation
Co-authored-by: Trevor <[email protected]>
| setHasError(false); | ||
| } else { | ||
| const loadTags = useCallback( | ||
| async (attempts = 10) => { |
There was a problem hiding this comment.
Is there a reason we chose to go with 10 attempts? I feel like most of the time we do 3 - 5 attempts for retries.
There was a problem hiding this comment.
it often can take more than 5 seconds to process the document
| @@ -33,6 +33,8 @@ import commonMessages from '../../common/messages'; | |||
| import './DocGenSidebar.scss'; | |||
| import { DocGenTag, DocGenTemplateTagsResponse, JsonPathsMap } from './types'; | |||
|
|
|||
| const DEFAULT_RETRIES = 10; | |||
There was a problem hiding this comment.
10 retries is a lot. How did we come up with this number? Would it be better to have have a timeout to determine whether or not to throw an error instead? Just a question.
There was a problem hiding this comment.
the time to process the document can take a while (in practice single digit number of seconds, but can take longer) - I can reduce the retries and increase the delay between them but that'd the time till content for the user longer.
| try { | ||
| const response: DocGenTemplateTagsResponse = await getDocGenTags(); | ||
| if (response.message) { | ||
| setTimeout(() => { |
There was a problem hiding this comment.
Are we doing this because we need to wait a second before trying to call the getDocGenTags again?
There was a problem hiding this comment.
i wanted to reduce number of calls - without it if the end user has fast internet connection they'd make additional calls - e.g. more than 1 call / second which seems unnecessary. e.g. if it takes 5 seconds to process the document, with this approach we're sending 5 requests, but without it if the request completes in 0.5 seconds we'd be sending 10 requests and so on. A small optimisation, not critical
| getDocGenTags: processingTagsMock, | ||
| }); | ||
|
|
||
| await jest.advanceTimersByTime(1000); |
There was a problem hiding this comment.
I'm assuming there's a reason why we can't put this in a loop?
There was a problem hiding this comment.
https://eslint.org/docs/latest/rules/no-await-in-loop - I tried to use Promise.all approach but it didn't work
rustam-e
dismissed stale reviews from abdel-ships-it and tjuanitas
via
c48a517
rustam-e
requested review from
JChan106,
bfoxx1906,
abdel-ships-it and
tjuanitas
before:
if the template has been updated and tags have not been processed - user would see error state requesting them to click a refresh button
after:
components tries re-fetching up to 10 times and only then shows the error state