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

chore(cloudflare): refactor structure, optimize patterns #8654

Conversation

alexanderniebuhr
Copy link
Member

@alexanderniebuhr alexanderniebuhr commented Sep 24, 2023

Changes

  • restructured: abstracted & moved code to make adapter more maintainable (this is a on-going process)
  • updated: reworded docs

Testing

  • existing tests

Docs

/cc @withastro/maintainers-docs for feedback!

@changeset-bot
Copy link

changeset-bot bot commented Sep 24, 2023

🦋 Changeset detected

Latest commit: 83499e6

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 24, 2023
@alexanderniebuhr alexanderniebuhr force-pushed the nbhr/chore_cloudflare_refactor_structure_optimize_patterns branch 2 times, most recently from a49d331 to 24d691a Compare September 24, 2023 07:21
@alexanderniebuhr alexanderniebuhr requested a review from a team September 24, 2023 08:06
@alexanderniebuhr alexanderniebuhr marked this pull request as ready for review September 24, 2023 08:33
@jadbox
Copy link
Contributor

jadbox commented Sep 24, 2023

@alexanderniebuhr very excited to see this work! This might be a dumb question, but it looks like there's a fair amount of work to get CF properly integrated for each service, and it feels that this might be hard to maintain if cloudflare makes service API changes. I wonder if you had explored ways either:

a) Try to reuse code by importing parts of Wrangler for environment setup and service mocks
b) Work with Wrangler repo to improve its ability to act as a startup engine for "astro dev" so that Astro local developers can benefit from using wrangler without having to actually build their Astro project to preview.

@alexanderniebuhr
Copy link
Member Author

@jadbox thank you so much for your message. Let me try to answer it as good as I can, but feel free to ask any additional details, if you wish.

[I]t looks like there's a fair amount of work to get CF properly integrated for each service, and it feels that this might be hard to maintain if Cloudflare makes service API changes. [...]

This is not true. There is some work which we need to do, but it is not to much, don't get mislead by this PR. I'd recommend that you look into the D1, R2, KV bindings PR.

a) Try to reuse code by importing parts of Wrangler for environment setup and service mocks
b) Work with Wrangler repo to improve its ability to act as a startup engine for "astro dev" so that Astro local developers can benefit from using wrangler without having to actually build their Astro project to preview.

We are actually reusing as much code as possible, we also have a open request to get more APIs exposed.
But you are right, the main goal is to avoid to rebuild the Astro project every time.

@alexanderniebuhr alexanderniebuhr force-pushed the nbhr/chore_cloudflare_refactor_structure_optimize_patterns branch from 24d691a to ecf0487 Compare September 25, 2023 08:38
@alexanderniebuhr alexanderniebuhr force-pushed the nbhr/chore_cloudflare_refactor_structure_optimize_patterns branch 5 times, most recently from 371e533 to 4459a36 Compare September 25, 2023 15:22
@jadbox
Copy link
Contributor

jadbox commented Sep 25, 2023

Hey @alexanderniebuhr, this may be out of scope, but I saw that Wrangler now injects a new AI binding that would be lovely to also have available.
https://github.com/cloudflare/workers-sdk/releases/tag/wrangler%403.9.1

@alexanderniebuhr
Copy link
Member Author

Wrangler now injects a new AI binding

@jadbox good to know! Do you know if it is the same as Cloudflare constellation?

Also please note that we currently focus on bindings which can be mocked locally, that is similar to using wrangler --local.

If you would need to run wrangler --remote for the AI binding to work, we will implement support in a second step! But we are already working on it.

@jadbox
Copy link
Contributor

jadbox commented Sep 25, 2023

@alexanderniebuhr the docs are not clear, but I think your right that it's just Constellation and will require a remote flag for binding. Good to know that remote services will come in the second phase! God speed on phase 1 and looking forward to testing 🫡

@alexanderniebuhr alexanderniebuhr force-pushed the nbhr/chore_cloudflare_refactor_structure_optimize_patterns branch 2 times, most recently from b1cb44a to 874a62b Compare September 26, 2023 07:11
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! I can see the new section added really incorporated some docs suggestions we've discussed before! 🙌

There's a bunch to go through here though (not all of it yours, but highlighted by the Wonderful World of Giff Diffs) so get ready for a real docs review incoming! 😄

packages/integrations/cloudflare/README.md Outdated Show resolved Hide resolved
packages/integrations/cloudflare/README.md Outdated Show resolved Hide resolved
packages/integrations/cloudflare/README.md Outdated Show resolved Hide resolved
packages/integrations/cloudflare/README.md Outdated Show resolved Hide resolved
packages/integrations/cloudflare/README.md Outdated Show resolved Hide resolved
packages/integrations/cloudflare/README.md Outdated Show resolved Hide resolved
packages/integrations/cloudflare/README.md Outdated Show resolved Hide resolved
packages/integrations/cloudflare/README.md Outdated Show resolved Hide resolved
packages/integrations/cloudflare/README.md Outdated Show resolved Hide resolved
packages/integrations/cloudflare/README.md Outdated Show resolved Hide resolved
@alexanderniebuhr alexanderniebuhr force-pushed the nbhr/chore_cloudflare_refactor_structure_optimize_patterns branch 3 times, most recently from 1a3d27f to 079acab Compare September 26, 2023 14:40
@alexanderniebuhr alexanderniebuhr force-pushed the nbhr/chore_cloudflare_refactor_structure_optimize_patterns branch from c34bcc4 to ab663a6 Compare September 26, 2023 17:11
@alexanderniebuhr alexanderniebuhr force-pushed the nbhr/chore_cloudflare_refactor_structure_optimize_patterns branch from ab663a6 to c96c643 Compare September 27, 2023 06:11
@alexanderniebuhr

This comment was marked as outdated.

@alexanderniebuhr alexanderniebuhr force-pushed the nbhr/chore_cloudflare_refactor_structure_optimize_patterns branch 2 times, most recently from 2388d0f to 274dc33 Compare September 27, 2023 07:38
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.

One more pass, and probably however you choose to handle these comments will be fine and not require another review by me! If you'd like me to have another look after you think you're finished, then just ping me again in docs-ptal, otherwise I'm confident you can run with these comments and you got this!

packages/integrations/cloudflare/README.md Outdated Show resolved Hide resolved
packages/integrations/cloudflare/README.md Outdated Show resolved Hide resolved
packages/integrations/cloudflare/README.md Outdated Show resolved Hide resolved
packages/integrations/cloudflare/README.md Outdated Show resolved Hide resolved
packages/integrations/cloudflare/README.md Outdated Show resolved Hide resolved
packages/integrations/cloudflare/README.md Outdated Show resolved Hide resolved
packages/integrations/cloudflare/README.md Outdated Show resolved Hide resolved
packages/integrations/cloudflare/README.md Outdated Show resolved Hide resolved
packages/integrations/cloudflare/README.md Outdated Show resolved Hide resolved
packages/integrations/cloudflare/README.md Outdated Show resolved Hide resolved
@alexanderniebuhr alexanderniebuhr force-pushed the nbhr/chore_cloudflare_refactor_structure_optimize_patterns branch 3 times, most recently from b6bc935 to 68a95a3 Compare September 27, 2023 14:20
@dario-piotrowicz
Copy link
Contributor

@alexanderniebuhr incredible stuff!!! 😍

I can't help but notice that DurableObjects aren't included in your set of PRs? is there a reason as to why?

@alexanderniebuhr
Copy link
Member Author

I can't help but notice that DurableObjects aren't included in your set of PRs? is there a reason as to why?

@dario-piotrowicz Thank you for your input. We're currently exploring the integration of DurableObjects. In this phase 1, we're prioritizing the addition of local bindings, analogous to the --local flag in wrangler.

However, I'm facing challenges in devising tests for these, primarily because a shared DurableObject isn't available during testing. If you have any suggestions or insights on how we can effectively test them, I'd be eager to share my preliminary merge request for inclusion.

@dario-piotrowicz
Copy link
Contributor

I can't help but notice that DurableObjects aren't included in your set of PRs? is there a reason as to why?

@dario-piotrowicz Thank you for your input. We're currently exploring the integration of DurableObjects. In this phase 1, we're prioritizing the addition of local bindings, analogous to the --local flag in wrangler.

However, I'm facing challenges in devising tests for these, primarily because a shared DurableObject isn't available during testing. If you have any suggestions or insights on how we can effectively test them, I'd be eager to share my preliminary merge request for inclusion.

Sorry I am not completely sure, I'm also wondering how you instantiate the DO for standard local development, which to me sounds like it should be as tricky, but that doesn't sound like an issue based on your message? 🤔

Anyways I've brought this up with the Miniflare author which will be much more suited then me to give you some insights/suggestions on what you can do 🙂

@mrbbot
Copy link

mrbbot commented Sep 28, 2023

Hey @alexanderniebuhr! 👋 Thanks for working on this. I have a few questions on the Durable Objects stuff:

However, I'm facing challenges in devising tests for these

Are you having trouble devising tests for Durable Objects in particular, or all the local bindings?

primarily because a shared DurableObject isn't available during testing

Which shared object are you referring to here? Is this an object you were hoping to implement just for the test? Or an object provided by an Astro user?

@alexanderniebuhr
Copy link
Member Author

alexanderniebuhr commented Sep 28, 2023

Are you having trouble devising tests for Durable Objects in particular, or all the local bindings?

Actually just Durable Objects, all other bindings work perfectly fine. Let's move the discussion to Discord.

Is this an object you were hoping to implement just for the test?

Exactly.

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 the refactor!

@alexanderniebuhr alexanderniebuhr force-pushed the nbhr/chore_cloudflare_refactor_structure_optimize_patterns branch from 68a95a3 to 8453a25 Compare September 28, 2023 12:58
@alexanderniebuhr alexanderniebuhr force-pushed the nbhr/chore_cloudflare_refactor_structure_optimize_patterns branch from 8453a25 to 798d018 Compare September 28, 2023 15:21
Refactors server directory structure by creating `entrypoints` directory and moving `server.advanced` and `server.directory` into it. Changes all respective imports to reflect this movement.

The server's code imports from `./util.js` are refactored to `../util.js` due to the server files' move into the `entrypoints` directory.

In `index.ts`, cleans up unused imports, and refactors code by moving utility functions that were previously inline to the new `./utils` directory.

The code refactoring and restructuring allows for easier navigation and enhancement. The server and utility functions separation follows the single responsibility principle, favoring modular design. The pattern optimization aids in a smoother deployment process by eliminating redundant patterns.

---------

Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca>
Co-authored-by: 100gle <loogle.space@gmail.com>
@alexanderniebuhr alexanderniebuhr force-pushed the nbhr/chore_cloudflare_refactor_structure_optimize_patterns branch from 798d018 to 83499e6 Compare September 28, 2023 15:47
@alexanderniebuhr alexanderniebuhr merged commit f6ba533 into main Sep 28, 2023
13 checks passed
@alexanderniebuhr alexanderniebuhr deleted the nbhr/chore_cloudflare_refactor_structure_optimize_patterns branch September 28, 2023 16:04
@astrobot-houston astrobot-houston mentioned this pull request Sep 28, 2023
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.

7 participants