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(@astrojs/netlify): Add on-demand builders Netlify functions #5874

Merged
merged 8 commits into from
Jan 27, 2023

Conversation

juanmiguelguerrero
Copy link
Contributor

@juanmiguelguerrero juanmiguelguerrero commented Jan 16, 2023

Changes

  • A new configuration option is added to the Netlify adapter to enable On-Demand Builder feature.

Testing

  • I don't know how to add a test with chai.
  • In any case, by adding this configuration, it is possible to test the functionality:
import { defineConfig } from 'astro/config';
import netlify from '@astrojs/netlify/functions';

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

Docs

/cc @withastro/maintainers-docs for feedback!

@changeset-bot
Copy link

changeset-bot bot commented Jan 16, 2023

🦋 Changeset detected

Latest commit: 39d5035

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 Jan 16, 2023
@matthewp
Copy link
Contributor

Very nice! Can you add a changeset? pnpm changeset, this would be a minor change since it's a new feature.

@matthewp matthewp requested a review from a team January 17, 2023 13:18
@juanmiguelguerrero
Copy link
Contributor Author

juanmiguelguerrero commented Jan 17, 2023

@matthewp There is a CI failure on macOS. It's related to a timeout in an Astro CLI test . I think this should not be impacted by this pull request. Thanks.

@matthewp
Copy link
Contributor

@juanmiguelguerrero Yeah thanks, I believe that's a flaky test. I'll rerun.

@juanmiguelguerrero
Copy link
Contributor Author

juanmiguelguerrero commented Jan 19, 2023

I found an error in the example code.

This is the correct code:

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

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

Note: I have edited the previous comment.

@sarah11918
Copy link
Member

@juanmiguelguerrero Here is the file that needs to be edited to add something to the docs configuration-reference page: https://github.com/withastro/astro/blob/main/packages/astro/src/%40types/astro.ts

Can you please document the necessary changes there, following the style of the other items at https://docs.astro.build/en/reference/configuration-reference/ ? Let me know if you need/want assistance with this.

Then the docs team will review them!

@juanmiguelguerrero
Copy link
Contributor Author

@sarah11918 I think there is nothing to change in that file.

This code changes do not affect the general Astro configuration. They only affect the Netlify adapter configuration.

The doc to update is this one: https://docs.astro.build/en/guides/integrations-guide/netlify/

https://github.com/withastro/docs/blob/099aba24730af21f81770302c0a6796163b11c4c/src/pages/en/guides/integrations-guide/netlify.mdx

In my opinion, the place to insert this change is inside the "Configuration" section, between the "dist" and "binaryMediaTypes" options.

capture

The text we should include would be something like this:

builders

On-demand Builders are serverless functions used to generate web content as needed that’s automatically cached on Netlify’s Edge CDN. To activate rendering with this type of functions, use the builders option:

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

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

This functionality is only available whith the @astrojs/netlify/functions adapter and is not compatible with Edge Functions.

For more information about Netlify On-demand Builders visit the Netlify documentation.

@sarah11918
Copy link
Member

@juanmiguelguerrero Gotcha! In that case, you'll want to edit this page: https://github.com/withastro/astro/blob/main/packages/integrations/netlify/README.md

This content is pulled into the Docs site from this README. Would you mind opening a PR with proposed changes there? Docs will get notification of that PR automatically, and that's where we can finalize the wording together. Thanks!

@juanmiguelguerrero
Copy link
Contributor Author

@sarah11918 I have just updated the documentation.

@juanmiguelguerrero
Copy link
Contributor Author

@matthewp Jhon, can you resolve these conflicts?

@sarah11918 Have you seen the changes I made to the README? Are they correct?

Can I help? Thanks... 🚀

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.

Thanks for the ping to let me know this was updated! Here are my suggestions for documentation text, if they don't lose your original intention!

packages/integrations/netlify/README.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
@juanmiguelguerrero
Copy link
Contributor Author

@matthewp I have already reviewed the pending conflicts.

@matthewp
Copy link
Contributor

Thanks @juanmiguelguerrero! This is looking great.

@sarah11918 just need your approval if there's nothing else.

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.

Docs is happy! Thanks @juanmiguelguerrero 🙌

@matthewp matthewp merged commit 1c230f1 into withastro:main Jan 27, 2023
This was referenced Jan 27, 2023
@betabong
Copy link

Netlify on-demand builders allow setting a Time to Live duration – this can be very useful for content like e-commerce or larger CMS setups. Ideally this could be configured per route (Astro.response). I was hoping that setting a header could accomplish it, but it seems like Netlify expects to have this set in the response. So may be "builders" could become a more complex option?

a. builder: true
b. builder: 3600
c. builder: (event, context) => event.path.startsWith("/archive") ? false : 60

@juanmiguelguerrero
Copy link
Contributor Author

@betabong It is an interesting functionality and is not difficult to implement.

My only doubt is how would be the best configuration option?

I think the simplest is something like this:

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

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

Anyway I would like to know before the opinion of @matthewp

@betabong
Copy link

Hey @juanmiguelguerrero I have made a pull request here #6219

Please have a look, thanks 🙏

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