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

Add Incremental Static Regeneration support for the Netlify's on-demand builders #7975

Merged
merged 13 commits into from
Aug 11, 2023

Conversation

lilnasy
Copy link
Contributor

@lilnasy lilnasy commented Aug 6, 2023

Changes

image
  • Adding types
image

Testing

Added a fixture
image

Docs

Added to the package README
image

@changeset-bot
Copy link

changeset-bot bot commented Aug 6, 2023

🦋 Changeset detected

Latest commit: e51d701

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions github-actions bot added the pkg: integration Related to any renderer integration (scope) label Aug 6, 2023
@lilnasy lilnasy changed the title Add Incremental Static Regeneration support for the Netliify adapter Add Incremental Static Regeneration support for the Netlify adapter Aug 6, 2023
@lilnasy lilnasy requested a review from a team as a code owner August 6, 2023 14:12
Copy link
Member

@ematipico ematipico left a comment

Choose a reason for hiding this comment

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

locals in an Astro component are read-only properties and you won't be able to set anything like this. The only place where you can write stuff in the locals is inside the middleware.

@natemoo-re
Copy link
Member

locals in an Astro component are read-only properties and you won't be able to set anything like this. The only place where you can write stuff in the locals is inside the middleware.

Ah, that is a very important limitation of this API! Should we use a Proxy during astro dev that throws an error when users attempt to assign a value via set()?

@lilnasy
Copy link
Contributor Author

lilnasy commented Aug 7, 2023

@ematipico It works. Do you mean it should be read-only?

@lilnasy
Copy link
Contributor Author

lilnasy commented Aug 7, 2023

There is no official word of this outside of Netlify's API reference, but most of on-demand builders' functionality has been brought into other functions.

I found this out from a Netlify support thread, and the gist of it is, its use case has become niche: single cache store used globally - instead of per-region, as it is for normal and edge functions.

This PR could still be merged for feature-completeness. I will be updating the Netlify part of the ISR guide to use Cache-Control headers instead.

@lilnasy lilnasy changed the title Add Incremental Static Regeneration support for the Netlify adapter Add Incremental Static Regeneration support for the Netlify's on-demand builders Aug 7, 2023
@ematipico
Copy link
Member

@ematipico It works. Do you mean it should be read-only?

@lilnasy

Yes. Astro.locals must be read-only. That's what dev and node adapter do in the first place. Making Astro.locals writeable in the netlify would cause different behaviour in the APIs. I understand that it works, but that's not how the APIs should be crafted.

locals are meant to be written in the middleware layer.

locals in an Astro component are read-only properties and you won't be able to set anything like this. The only place where you can write stuff in the locals is inside the middleware.

Ah, that is a very important limitation of this API! Should we use a Proxy during astro dev that throws an error when users attempt to assign a value via set()?

We don't use Proxy in core, but yeah, that's another way to do it.

@lilnasy
Copy link
Contributor Author

lilnasy commented Aug 8, 2023

@ematipico Considering that serializability restrictions are relaxed, maybe a setTtl() function is more appropriate.

---
Astro.locals.netlify.builders.setTtl(45)
---
<h1>Astro</h1>

@ematipico
Copy link
Member

@ematipico Considering that serializability restrictions are relaxed, maybe a setTtl() function is more appropriate.

---
Astro.locals.netlify.builders.setTtl(45)
---
<h1>Astro</h1>

Yeah that's way better I'd say

@lilnasy
Copy link
Contributor Author

lilnasy commented Aug 9, 2023

I would like a pointer for adding a no-op for dev.

I see dev mode support was added in #7385, but the two tests added in it are for build.

@ematipico
Copy link
Member

ematipico commented Aug 9, 2023

I would like a pointer for adding a no-op for dev.

I see dev mode support was added in #7385, but the two tests added in it are for build.

Yeah, the PR title is misleading (it was picked up from a third-party contributor).

I am not sure how we can fix this case, with the code we have now. I think Astro should provide a safe way to retrieve the "adapter runtime", and if the adapter doesn't provide any runtime, it should be undefined.

E.g.

const runtime = Astro.locals.getAdapterRuntime();
if (runtime) {
	runtime.setBuilderTtl(45)
}

The downside is that these kinds of things are only visible when building the project and deploying it.

@lilnasy
Copy link
Contributor Author

lilnasy commented Aug 10, 2023

Should I add a import.meta.env.PROD check in the docs for now?

Copy link
Member

@ematipico ematipico left a comment

Choose a reason for hiding this comment

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

Looks good!

Copy link
Member

@ElianCodes ElianCodes left a comment

Choose a reason for hiding this comment

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

A couple of small changes, but after applying, good to go!

.changeset/sweet-cows-roll.md Outdated Show resolved Hide resolved
packages/integrations/netlify/README.md Outdated Show resolved Hide resolved
packages/integrations/netlify/README.md Outdated Show resolved Hide resolved
Copy link
Member

@ElianCodes ElianCodes left a comment

Choose a reason for hiding this comment

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

Good to go!

@lilnasy lilnasy merged commit f974c95 into main Aug 11, 2023
1 check passed
@lilnasy lilnasy deleted the netlify-isr branch August 11, 2023 14:09
@astrobot-houston astrobot-houston mentioned this pull request Aug 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: integration Related to any renderer integration (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add TTL support for Netlify adapter for On Demand Builders
6 participants