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(vercel): ISR #6739

Merged
merged 11 commits into from
Feb 8, 2024
Merged

feat(vercel): ISR #6739

merged 11 commits into from
Feb 8, 2024

Conversation

lilnasy
Copy link
Contributor

@lilnasy lilnasy commented Feb 1, 2024

Description (required)

  • Adds configuration reference for a new option being introduced in the next minor version.
  • Brief; detailed instructions would likely go on vercel docs instead.

Related issues & labels (optional)

Copy link

vercel bot commented Feb 1, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
docs ✅ Ready (Inspect) Visit Preview Feb 7, 2024 1:18pm
1 Ignored Deployment
Name Status Preview Updated (UTC)
docs-i18n ⬜️ Ignored (Inspect) Feb 7, 2024 1:18pm

Copy link
Contributor

@VoxelMC VoxelMC left a comment

Choose a reason for hiding this comment

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

From what I can see, it looks great! Nice work :)
LGTM

@lilnasy lilnasy mentioned this pull request Feb 1, 2024
6 tasks
@lilnasy
Copy link
Contributor Author

lilnasy commented Feb 1, 2024

Thanks 😊

@sarah11918 sarah11918 added add new content Document something that is not in docs. May require testing, confirmation, or affect other pages. merge-on-release Don't merge this before the feature is released! (MQ=approved but WAIT for feature release!) labels Feb 5, 2024
@sarah11918
Copy link
Member

Quick q so I can prioritize, @lilnasy - I see you made the implementation PR into a draft last week. Would anything there that you might rework change this user-facing documentation, or is this content safe to review? Thanks!

@lilnasy
Copy link
Contributor Author

lilnasy commented Feb 5, 2024

It is safe to review. The reason for making it draft was resolved soon after.

@sarah11918 sarah11918 added the minor-release For the next minor release; in the milestone, "merge queue" when approved by Sarah! label Feb 5, 2024
@sarah11918
Copy link
Member

This is going to need all that good stuff from the other PR's changeset!

@lilnasy
Copy link
Contributor Author

lilnasy commented Feb 5, 2024

This is going to need all that good stuff from the other PR's changeset!

From what I've seen with other frameworks, vercel documents ISR in more depth on their site. SvelteKit, for example. We have an opportunity to keep the changes here minimal and link to Astro on Vercel when it updates.

How much should we exercise this opportunity?

@sarah11918
Copy link
Member

sarah11918 commented Feb 5, 2024

We need to document the configuration options! our stuff: expiration etc.

Unless, you're telling me those are all Vercel options that you can throw in your vercel({}) config. In which case, I'd want to reframe the changeset in the other PR to imply:

Use any of the config options here, for example to control cache invalidation...

rather than make it look like these are particular @astrojs/vercel config properties.

@lilnasy
Copy link
Contributor Author

lilnasy commented Feb 5, 2024

I am trying to figure out the criteria of what goes in the readme. Should VercelImageConfig have been documented as well?

@sarah11918
Copy link
Member

My general rule is: anything that we built, literally anything you can configure or use that only we control, we should document.

If, for example (as I think was once the case with something in an adapter or middleware or something), we use/support someone else's entire options (we exactly mirror them), then a link to them would be sufficient.

I don't know what VercelImageConfig is, but if it is something we created, even if only in order to use their image service, then yes we should document it. BUT, for example, if all the config options are just mirrored from their service, that's where it's fine to link to them.

Does that help?

@lilnasy
Copy link
Contributor Author

lilnasy commented Feb 5, 2024

That helps, thanks!

I've documented VercelISRConfig.

I don't have the full context for VercelImageConfig but it is our config, and not present on docs. It can be a future PR, possibly by someone else.

Copy link
Member

@delucis delucis left a comment

Choose a reason for hiding this comment

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

Nice work @lilnasy! Left a couple of comments.

Co-authored-by: Chris Swithinbank <[email protected]>
Copy link
Member

@sarah11918 sarah11918 left a comment

Choose a reason for hiding this comment

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

Some changes suggested to bring the readability up to that of the changeset from the other PR!

(No paragraphs inside code samples, plz! 😅 )

@lilnasy
Copy link
Contributor Author

lilnasy commented Feb 6, 2024

@sarah11918 Thanks, this is much clearer now!

Copy link
Member

@dreyfus92 dreyfus92 left a comment

Choose a reason for hiding this comment

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

Readibility was improved by 1000%, thank you @lilnasy for tackling this and everyone else. LGTM 😁

Copy link
Member

@delucis delucis left a comment

Choose a reason for hiding this comment

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

Lovely work @lilnasy — I think this looks great 🙌

Tiny final suggestion to highlight the isr options in a couple of code blocks but basically LGTM 🚀

Co-authored-by: Chris Swithinbank <[email protected]>
@lilnasy lilnasy merged commit 44c3650 into main Feb 8, 2024
7 checks passed
@lilnasy lilnasy deleted the isr branch February 8, 2024 13:36
Copy link
Member

@sarah11918 sarah11918 left a comment

Choose a reason for hiding this comment

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

Thank you @lilnasy for your great work getting this into docs format! Ready for you to merge at will! 🫡

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
add new content Document something that is not in docs. May require testing, confirmation, or affect other pages. merge-on-release Don't merge this before the feature is released! (MQ=approved but WAIT for feature release!) minor-release For the next minor release; in the milestone, "merge queue" when approved by Sarah!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants