Skip to content
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

Modified retrieval of basic/advanced headers #24

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

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