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 ttl option to netlify adapter for builders #6219

Closed

Conversation

betabong
Copy link

Changes

#5874 added an option to render astro pages via Netlify On-Demand Builders.

  • Extends new builders option for Netlify adapter
  • Allows to set a TTL (Time to Live) option:
    • As a number for all pages / routes
    • As a function per request
// astro.config.mjs
import { defineConfig } from 'astro/config';
import netlify from '@astrojs/netlify/functions';

export default defineConfig({
  output: 'server',
  adapter: netlify({
    builders: {
      ttl(event) {
        return event.path.startsWith('/blog') ? 3600 : undefined;
      },
    },
  }),
});

Testing

I honestly don't know how to test this. I was not even able to try it out by myself, as I don't know how to link a git subpackage as a dependency. If anybody can hint me on how to do this, I'm happy to test things with an actual project.

Docs

@withastro/maintainers-docs I have updated the Readme with instructions.

@betabong betabong requested a review from a team as a code owner February 11, 2023 11:51
@changeset-bot
Copy link

changeset-bot bot commented Feb 11, 2023

⚠️ No Changeset found

Latest commit: a74918e

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

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

@github-actions github-actions bot added the pkg: integration Related to any renderer integration (scope) label Feb 11, 2023
Copy link
Member

@yanthomasdev yanthomasdev left a comment

Choose a reason for hiding this comment

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

Thanks for this @betabong, I added a few docs related suggestions, let me know your thoughts! 🙌

}),
});
```

On-demand Builders are only available with the `@astrojs/netlify/functions` adapter and are not compatible with Edge Functions.
On-demand Builders optionally let you return a [TTL](https://docs.netlify.com/configure-builds/on-demand-builders/#time-to-live-ttl) (time-to-live) value for the cached page. The astro adapter allows you to set a default TTL value for all pages, or a custom TTL value for specific pages via a function call.
Copy link
Member

Choose a reason for hiding this comment

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

A few wording suggestions:

  1. I believe that "Astro adapter" is redundant, considering the context it's clear we're talking about the current adapter, so I would remove "Astro".
  2. We're repeating "TTL value" a few times in this sentence, what do you think of simplifying it a bit?
Suggested change
On-demand Builders optionally let you return a [TTL](https://docs.netlify.com/configure-builds/on-demand-builders/#time-to-live-ttl) (time-to-live) value for the cached page. The astro adapter allows you to set a default TTL value for all pages, or a custom TTL value for specific pages via a function call.
On-demand Builders optionally let you return a [TTL](https://docs.netlify.com/configure-builds/on-demand-builders/#time-to-live-ttl) (time-to-live) value for the cached page. The adapter allows you to set a default TTL value for all pages or a custom one for specific pages via a function call.


On-demand Builders are only available with the `@astrojs/netlify/functions` adapter and are not compatible with Edge Functions.
Copy link
Member

Choose a reason for hiding this comment

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

The sentence above also starts with "On-demand Builders" and I think we should call a bit more attention to it, so I changed it a bit adding "Note that" so people notice this part more!

Suggested change
On-demand Builders are only available with the `@astrojs/netlify/functions` adapter and are not compatible with Edge Functions.
Note that On-demand Builders are only available with the `@astrojs/netlify/functions` adapter and are not compatible with Edge Functions.

Copy link
Author

Choose a reason for hiding this comment

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

@Yan-Thomas Great suggestions, thanks 🙏

@juanmiguelguerrero
Copy link
Contributor

@betabong I think it would be good to add an example where the ttl is a number. Reading the new doc it's not quite clear how it should be done.

// astro.config.mjs
import { defineConfig } from 'astro/config';
import netlify from '@astrojs/netlify/functions';

export default defineConfig({
  output: 'server',
  adapter: netlify({
    builders: {
      ttl: 3600
      },
    },
  }),
});

builders?:
| boolean
| {
ttl?: number | ((event: HandlerEvent) => number | undefined);
Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately this kind of API won't work. Args gets serialized and included in the build. Your astro.config.mjs does not get bundled with the build. So only plain objects/primitives can be passed into the actual integration. Functions won't work.

How we usually fix this is by instead having the code be defined in a separate module and then building that module as part of the build. Something: builders: 'path/to/implementation.ts'. It's not as nice of an API but it prevents us from needing to build/bundle the config file

Copy link
Author

Choose a reason for hiding this comment

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

@matthewp Oh wow, I wasn't aware of that. Does this only apply to adapter configurations? After all we can use Vite plugins in astro.config that again are (partly) functions: https://docs.astro.build/en/reference/configuration-reference/#vite – on the other hand I can't find any example of passing a path to .ts or .js file. If you know of any other usage, that would be very helpful indeed.

Copy link
Contributor

Choose a reason for hiding this comment

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

We don't build the config, so any functions that need to exist in production cannot be provided via a config option.

As for examples, all of the renderers work this way, see for example:

return {
name: '@astrojs/vue',
clientEntrypoint: '@astrojs/vue/client.js',
serverEntrypoint: '@astrojs/vue/server.js',
};

I'd suggest a similar pattern if you want to allow the user to customize runtime values.

@matthewp
Copy link
Contributor

matthewp commented Mar 7, 2023

Closing as the current approach doesn't work with SSR. You could just omit the function callback approach, which would be serializable. Or go with a pattern where the user passes a path to a file that gets built. I'll leave that decision up to you.

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.

4 participants