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/cloudflare): add runtime support to astro dev #8426

Merged
merged 21 commits into from
Sep 11, 2023
Merged

feat(@astrojs/cloudflare): add runtime support to astro dev #8426

merged 21 commits into from
Sep 11, 2023

Conversation

alexanderniebuhr
Copy link
Member

@alexanderniebuhr alexanderniebuhr commented Sep 6, 2023

Changes

  • clean up: there were still some references to old getRuntime
  • workaround: until wrangler exposes for of their API for programatically usage, I just copied over the important logic
  • add: req.cf or Astro.locals.runtime.cf support for astro dev, without the need to run wrangler
  • add: Astro.locals.runtime.env support for enviromental vars defined according to CF docs in either wrangler.toml or .dev.vars

Known issues (for another PR)

  • missing: CF bindings mocking (R2, D1, KV, Durable Objects)
  • missing: support for env vars defined in other files than wrangler.toml, .dev.vars
  • missing: wrangler.json support

Testing

  • manually

Docs

- would love to add them with a later PR, just want to stealth release it first

/cc @withastro/maintainers-docs for feedback!

@changeset-bot
Copy link

changeset-bot bot commented Sep 6, 2023

🦋 Changeset detected

Latest commit: 671bb24

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 Sep 6, 2023
@alexanderniebuhr alexanderniebuhr changed the title Alexanderniebuhr/astro dev support cf runtime feat(@astrojs/cloudflare): add runtime support to astro dev Sep 6, 2023
lilnasy
lilnasy previously requested changes Sep 6, 2023
Copy link
Contributor

@lilnasy lilnasy left a comment

Choose a reason for hiding this comment

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

Love this! Setting the standard for adapters yet again.

As important as documentation is for this feature, it could be deferred. But there should at least be basic tests. If for nothing else, then ensuring that things don't wildly change.

packages/integrations/cloudflare/src/index.ts Outdated Show resolved Hide resolved
packages/integrations/cloudflare/src/index.ts Outdated Show resolved Hide resolved
packages/integrations/cloudflare/src/index.ts Outdated Show resolved Hide resolved
packages/integrations/cloudflare/src/index.ts Outdated Show resolved Hide resolved
packages/integrations/cloudflare/src/index.ts Show resolved Hide resolved
packages/integrations/cloudflare/src/index.ts Show resolved Hide resolved
packages/integrations/cloudflare/src/CFVars.ts Outdated Show resolved Hide resolved
packages/integrations/cloudflare/src/CFVars.ts Outdated Show resolved Hide resolved
packages/integrations/cloudflare/src/CFVars.ts Outdated Show resolved Hide resolved
alexanderniebuhr and others added 9 commits September 6, 2023 17:27
Co-authored-by: Arsh <69170106+lilnasy@users.noreply.github.com>
Co-authored-by: Arsh <69170106+lilnasy@users.noreply.github.com>
Co-authored-by: Arsh <69170106+lilnasy@users.noreply.github.com>
Co-authored-by: Arsh <69170106+lilnasy@users.noreply.github.com>
Co-authored-by: Arsh <69170106+lilnasy@users.noreply.github.com>
@alexanderniebuhr
Copy link
Member Author

@lilnasy, thank you for your insightful comments! You're correct that I included code comments intended for a later stage. I agree with your suggestion to remove them for the time being; they can always be reintroduced later.

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.

Code-wise, it looks good. I agree with every comment @lilnasy made

We should add some documentation once we settle the business logic

packages/integrations/cloudflare/src/CFVars.ts Outdated Show resolved Hide resolved
packages/integrations/cloudflare/src/index.ts Outdated Show resolved Hide resolved
packages/integrations/cloudflare/src/index.ts Outdated Show resolved Hide resolved
@alexanderniebuhr
Copy link
Member Author

Fixed most of the comments. I actually implemented local caches support, using miniflare. Will add some tests & docs later today.

@alexanderniebuhr alexanderniebuhr requested a review from a team as a code owner September 11, 2023 12:50
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 @alexanderniebuhr! Just some docs tightening up to do, and I've given you a structure that you can adjust as necessary! 🙌

packages/integrations/cloudflare/README.md Outdated Show resolved Hide resolved
packages/integrations/cloudflare/README.md Outdated Show resolved Hide resolved
@lilnasy lilnasy dismissed their stale review September 11, 2023 14:05

The tests look great! I'm going to try the dev changes live later.

Copy link
Contributor

@matthewp matthewp left a comment

Choose a reason for hiding this comment

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

Love this

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.

Great work!

Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca>
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.

It's been a pleasure doing doc'ing with you! 🫡

Copy link
Contributor

@lilnasy lilnasy left a comment

Choose a reason for hiding this comment

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

This is great! Thanks for all the hardwork you put in here.

@lilnasy lilnasy merged commit 2c96144 into withastro:main Sep 11, 2023
13 checks passed
@astrobot-houston astrobot-houston mentioned this pull request Sep 11, 2023
@alexanderniebuhr alexanderniebuhr deleted the alexanderniebuhr/astro-dev-support-cf-runtime branch September 23, 2023 05:34
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.

5 participants