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

[Pages] update bindings in your Astro application #10881

Merged
merged 2 commits into from
Oct 3, 2023

Conversation

ntsd
Copy link
Contributor

@ntsd ntsd commented Sep 26, 2023

• update astro binding docs since @astrojs/cloudflare/runtime is deprecate
• reference type for @cloudflare/workers-types
• add access kv in astro component example
• add learn more about Cloudflare runtime link to Astro doc

Copy link
Contributor

@alexanderniebuhr alexanderniebuhr left a comment

Choose a reason for hiding this comment

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

@ntsd you were faster than me. Thanks for that!
I'm the Astro maintainer, who was developing the change. I'll just added some notes!

content/pages/framework-guides/deploy-an-astro-site.md Outdated Show resolved Hide resolved
content/pages/framework-guides/deploy-an-astro-site.md Outdated Show resolved Hide resolved
content/pages/framework-guides/deploy-an-astro-site.md Outdated Show resolved Hide resolved
@ntsd ntsd force-pushed the docs/fix-astro-bind branch from 43dc46c to d60e552 Compare September 26, 2023 21:21
@ntsd ntsd marked this pull request as draft September 26, 2023 21:23
@ntsd ntsd force-pushed the docs/fix-astro-bind branch from d60e552 to 4dfbedd Compare September 26, 2023 21:57
@ntsd ntsd marked this pull request as ready for review September 26, 2023 22:01
@WalshyDev WalshyDev requested review from dario-piotrowicz and alexanderniebuhr and removed request for alexanderniebuhr September 26, 2023 22:10
@ntsd ntsd force-pushed the docs/fix-astro-bind branch 4 times, most recently from aaf2114 to bc3b8a8 Compare September 27, 2023 06:44
Copy link
Member

@dario-piotrowicz dario-piotrowicz left a comment

Choose a reason for hiding this comment

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

Thanks a lot @ntsd! 😃 (and thanks a lot @alexanderniebuhr for your help as well of course! 😄)

I think that technically the additions all look great, but I think we have a problem with the flow of the bindings section

Here is how the section looks like with the code removed:
Screenshot at 2023-10-01 13-10-41

I personally think that this reads quite poorly and doesn't have a clear flow (we start mentioning bindings as we do for all guides, then we move to Astro endpoints (which is ok), but then jump stright to the env.d.ts file, then implement an endpoint and at the end mention Astro components as an aftertought), I think that we should update it to a have a structure along the lines of:

A binding allows your application to interact with Cloudflare developer products, [...]

In Astro you can use bindings in Astro components and API routes by using the `context.local` [...]

Let's see an example with typescript:

First we need to update the `env.d.ts`: [...]

And then we can use the KV in our Astro component: [...]

and in our API route: [...]

What do you think?

@@ -112,27 +112,61 @@ export default defineConfig({

A [binding](/pages/platform/functions/bindings/) allows your application to interact with Cloudflare developer products, such as [KV](/workers/learning/how-kv-works/), [Durable Object](/durable-objects/), [R2](/r2/), and [D1](https://blog.cloudflare.com/introducing-d1/).

In Astro you can add server-side code via [endpoints](https://docs.astro.build/en/core-concepts/endpoints/), in such endpoints you can then use the `getRuntime()` method to access Cloudflare's environment and consecutively any bindings set for your application.
In Astro you can add server-side code via [endpoints](https://docs.astro.build/en/core-concepts/endpoints/), in such endpoints you can then use the `context.local` from [Astro Middleware](https://docs.astro.build/en/guides/middleware/) to access Cloudflare's environment and consecutively any bindings set for your application.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
In Astro you can add server-side code via [endpoints](https://docs.astro.build/en/core-concepts/endpoints/), in such endpoints you can then use the `context.local` from [Astro Middleware](https://docs.astro.build/en/guides/middleware/) to access Cloudflare's environment and consecutively any bindings set for your application.
In Astro you can add server-side code via [endpoints](https://docs.astro.build/en/core-concepts/endpoints/), in such endpoints you can then use the `context.local` from [Astro Middleware](https://docs.astro.build/en/guides/middleware/) to access the Cloudflare runtime which amongst other fields contains the Cloudflare's environment and consecutively any bindings set for your application.

But I am not 100% sure, I am wondering if mentioning the runtime part makes things more clear or just adds more noise 🤔

content/pages/framework-guides/deploy-an-astro-site.md Outdated Show resolved Hide resolved
content/pages/framework-guides/deploy-an-astro-site.md Outdated Show resolved Hide resolved
content/pages/framework-guides/deploy-an-astro-site.md Outdated Show resolved Hide resolved
@dario-piotrowicz
Copy link
Member

PS: why I think that a restructure is needed here, before I think that the bindings were only accessible in endpoints so that's why we were only mentioning those and the section was ok, but now that bindings are also available in Astro components then I think that the section needs to be a bit restructured because it's very much endpoints-based (and again it feels all a bit assembled together without a good structure).

@ntsd ntsd force-pushed the docs/fix-astro-bind branch 5 times, most recently from 6c93f58 to bde109e Compare October 2, 2023 04:11
@ntsd ntsd requested a review from dario-piotrowicz October 2, 2023 04:11
@ntsd
Copy link
Contributor Author

ntsd commented Oct 2, 2023

Hi @dario-piotrowicz ,

I did update the PR following your suggestion, thanks a lot.

Actually, I think this includes the Cloudflare runtime functions, I'm not sure that Binding or Cloudflare runtime is the bigger topic.

Maybe a structure like this, I can write all if you want.

Access to Cloudflare Runtime
- Environment Binding
- Get incoming request data
- Caches

Copy link
Member

@dario-piotrowicz dario-piotrowicz left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the changes @ntsd, looks good to me 😄

(I've just left a couple of minor suggestions)

content/pages/framework-guides/deploy-an-astro-site.md Outdated Show resolved Hide resolved
content/pages/framework-guides/deploy-an-astro-site.md Outdated Show resolved Hide resolved
@dario-piotrowicz
Copy link
Member

Hi @dario-piotrowicz ,

I did update the PR following your suggestion, thanks a lot.

Actually, I think this includes the Cloudflare runtime functions, I'm not sure that Binding or Cloudflare runtime is the bigger topic.

Maybe a structure like this, I can write all if you want.

Access to Cloudflare Runtime
- Environment Binding
- Get incoming request data
- Caches

Yeah good point @ntsd, thanks for bringing it up 🙂

The bindings section is there as that is like the basic/fundamental piece that generally you'd probably want to know about, and most modern framework guides here do have such a section.
I do agree that having an explanation of the rest of the Cloudflare runtime would be beneficial, but I am not sure if these guides should include various details or they should just refer back to the official Astro docs pages (as in, adding a To learn more about the Astro Cloudflare runtime see: https://docs.astro.build/en/guides/integrations-guide/cloudflare/#access-to-the-cloudflare-runtime sentence and making sure that the Astro docs mention everything that's needed).

I'm not really sure, I'd be keen to see what @deadlypants1973 thinks.
(Thinking about it, maybe by following such logic maybe we should even not include binding sections in any or our guides but just always refer back to the official framework docs? 🤔)

(In any case I think we could/should merge this PR as is (plus PCX tweaks if needed) and potentially handle the above in a followup PR)

@alexanderniebuhr
Copy link
Contributor

alexanderniebuhr commented Oct 2, 2023

Not sure, but in case it's relevant, https://github.com/withastro/astro/pull/8655/files will land in the next release (prob, this week)

refer back to the official Astro docs pages (as in, adding a To learn more about the Astro Cloudflare runtime see: https://docs.astro.build/en/guides/integrations-guide/cloudflare/#access-to-the-cloudflare-runtime sentence and making sure that the Astro docs mention everything that's needed).

@dario-piotrowicz I agree, it would be up to date 99% of the time. The Astro team can only maintain their own docs, while we try to open PRs for 3rd party docs, it's hard to keep track of them & update them everytime.

@ntsd ntsd force-pushed the docs/fix-astro-bind branch from bde109e to fde8b91 Compare October 2, 2023 15:04
@ntsd
Copy link
Contributor Author

ntsd commented Oct 2, 2023

@dario-piotrowicz @alexanderniebuhr Agree with adding the link to Astro docs. I updated the PR by adding the last sentence sent to the Astro doc.

• update astro binding docs since `@astrojs/cloudflare/runtime` is deprecate
• fix binding in Astro comments
• reference type for `@cloudflare/workers-types`
• add access kv in astro component example
• add learn more about the Cloudflare runtime link to Astro doc

Co-authored-by: Alexander Niebuhr <[email protected]>
Co-authored-by: Dario Piotrowicz <[email protected]>
@ntsd ntsd force-pushed the docs/fix-astro-bind branch from fde8b91 to c0478c3 Compare October 2, 2023 15:09
@deadlypants1973 deadlypants1973 merged commit 00c3021 into cloudflare:production Oct 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants