-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
feat(@astrojs/cloudflare): add runtime support to astro dev
#8426
Conversation
🦋 Changeset detectedLatest 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 |
astro dev
There was a problem hiding this 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.
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>
@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. |
There was a problem hiding this 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
Fixed most of the comments. I actually implemented local |
There was a problem hiding this 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! 🙌
The tests look great! I'm going to try the dev changes live later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love this
There was a problem hiding this 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>
There was a problem hiding this 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! 🫡
There was a problem hiding this 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.
Changes
getRuntime
req.cf
orAstro.locals.runtime.cf
support forastro dev
, without the need to run wranglerAstro.locals.runtime.env
support for enviromental vars defined according to CF docs in eitherwrangler.toml
or.dev.vars
Known issues (for another PR)
wrangler.toml
,.dev.vars
wrangler.json
supportTesting
Docs
- would love to add them with a later PR, just want to stealth release it first/cc @withastro/maintainers-docs for feedback!