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

feat(prerender): support retry and retryDelay #1534

Merged
merged 15 commits into from
Oct 5, 2023
Merged

Conversation

Hebilicious
Copy link
Contributor

@Hebilicious Hebilicious commented Aug 4, 2023

πŸ”— Linked issue

Resolves #1469

❓ Type of change

  • πŸ“– Documentation (updates to the documentation, readme, or JSdoc annotations)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • 🧹 Chore (updates to the build process or auxiliary tools and libraries)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

The prerender by default will fail if a page fails to render. This PR changes the default behaviour to 3 attempts before failure.
This also adds 2 configuration options nitro.options.prerender.tries and nitro.options.prerender.retryDelay to change the behaviour. tries can be set toInifinityif someones wants the pre-render to try endlessly.

For the default values, I've arbitrarily chose 2 retries and a 250ms retry delay.

Additional thoughts

We could nest the options: nitro.options.prerender.retry.attempts and nitro.options.prerender.retry.delay.

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

@codecov
Copy link

codecov bot commented Aug 4, 2023

Codecov Report

Merging #1534 (ab2be85) into main (b42a432) will decrease coverage by 0.05%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1534      +/-   ##
==========================================
- Coverage   76.43%   76.38%   -0.05%     
==========================================
  Files          76       76              
  Lines        8008     8025      +17     
  Branches      810      806       -4     
==========================================
+ Hits         6121     6130       +9     
- Misses       1885     1892       +7     
- Partials        2        3       +1     
Files Coverage Ξ”
src/options.ts 96.31% <100.00%> (+0.01%) ⬆️
src/prerender.ts 86.11% <100.00%> (+0.16%) ⬆️
src/types/nitro.ts 100.00% <100.00%> (ΓΈ)

... and 2 files with indirect coverage changes

@pi0
Copy link
Member

pi0 commented Aug 4, 2023

Since we use ofetch, i am wondering if we can make use of retry feature. Also was thinking to call it retry / retries because it is additional attempts (and 0 means once as default). tries: 0 has no meaning.

@Hebilicious
Copy link
Contributor Author

Hebilicious commented Aug 4, 2023

Since we use ofetch, i am wondering if we can make use of retry feature. Also was thinking to call it retry / retries because it is additional attempts (and 0 means once as default). tries: 0 has no meaning.

The ofetch implementation doesn't support a configurable delay. Should I use it regardless ?
I will refactor to retries instead of tries.

@pi0
Copy link
Member

pi0 commented Aug 4, 2023

The ofetch implementation doesn't support a configurable delay. Should I use it regardless ?

I am positive to include it upstream. PR also welcome πŸ‘πŸΌ

@pi0 pi0 marked this pull request as draft August 6, 2023 18:25
@pi0 pi0 changed the title feat: implement pre-render tries feat(prerender): support retry Aug 6, 2023
@Hebilicious
Copy link
Contributor Author

Hebilicious commented Aug 8, 2023

Tagging as pending until unjs/ofetch#262 is merged

@rudolfbyker
Copy link

Thanks for working on this!

Just my two cents about tries vs retries: #1469 (comment) Indeed, tries: 0 does not make sense. That should be an error, just like tries: -1 or tries: "foo". However, it's still less confusing to have tries: 1, tries: 2, etc. than retries: 0, retries: 1, etc. If you feel this is nit-picking, just ignore me :)

@Hebilicious
Copy link
Contributor Author

@pi0 I've rebased this with latest ofetch πŸ‘πŸ½

src/prerender.ts Outdated Show resolved Hide resolved
pi0 and others added 2 commits October 5, 2023 20:22
(we can introduce alias later to ofetch first if makes better sense)
@pi0 pi0 changed the title feat(prerender): support retry feat(prerender): support retry and retryDelay Oct 5, 2023
Copy link
Member

@pi0 pi0 left a comment

Choose a reason for hiding this comment

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

❀️

@pi0 pi0 merged commit a584068 into main Oct 5, 2023
9 checks passed
@pi0 pi0 deleted the feat/retry-prerender branch October 5, 2023 18:33
@pi0 pi0 mentioned this pull request Oct 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request prerender
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Retry failed routes during prerendering
4 participants