Skip to content

Conversation

@Rezer
Copy link
Contributor

@Rezer Rezer commented May 8, 2022

Added up to 10 retries for basicHeaders and advancedHeaders within handle-scheduled-checkin.ts

Increased puppeteer timeout to 60s within sw-generate-headers.ts

Resolves #23

Added up to 10 retries for basicHeaders and advancedHeaders within handle-scheduled-checkin.ts

Increased puppeteer timeout to 60s within sw-generate-headers.ts
@swtools0
Copy link
Contributor

swtools0 commented May 9, 2022

This looks like a terrific update, but I have a few suggestions about the implementation:

  1. Let's use a simple for loop for the retry logic rather than a while(true) loop. We can iterate until either the loop is done (max retries reached) or the headers successfully found.
  2. I would remove the 60s puppeteer timeout. In fact, we might want to decrease it rather than increase it now that we are retrying. I would assume that if it's taking longer than like 10s to load the page, that page is never going to load. Let me know if you observed in your testing that the page sometimes takes longer than 30s to load, but then actually successfully loads if it has 60s to wait.
  3. This project uses prettier and eslint. If you use VSCode, just install the project's recommended extensions, reload the window, and save your files. They should be auto-formatted by prettier.

if(maxRetries <= 0)
throw error;
console.debug(`Failed getting ${headerName}. Retrying in 5s...\nException details: ${error}`)
await waitMs(5000);
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a comment about why we are waiting

}
catch(error){
if(maxRetries <= 0)
throw error;
Copy link
Contributor

@swtools0 swtools0 May 9, 2022

Choose a reason for hiding this comment

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

Rather than throwing an error, just return here, then update the caller to check if the result is truthy. The handleInternal function should get to decide how to deal with headers not being found (probably throw an error there).

Copy link
Contributor

@swtools0 swtools0 left a comment

Choose a reason for hiding this comment

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

Left feedback

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.

Puppeteer timeout on checkin

2 participants