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(cloudflare): experimental config redirection support #2949

Draft
wants to merge 5 commits into
base: v2
Choose a base branch
from

Conversation

pi0
Copy link
Member

@pi0 pi0 commented Dec 20, 2024

Nitro has introduced an experimental (opt-in) way to generate/extend wrangler.toml for cloudflare deployments (#2353, #2355).

The end goal is to leverage Cloudflare platform features such as Node compat v2 (hybrid) and Async Context with zero-config and ensure we can reliably enable these features.

However, overwriting the user config is not ideal or recommended.

Thanks to the huge huge efforts and collab with the Cloudflare team and @petebacondarwin, a new solution (~> cloudflare/workers-sdk#7442) is coming to allow frameworks like Nitro to generate a merged wrangler.toml in dist and config that redirects/points wrangler CLI and pages CI to it.

This PR is to experiment this new feature e2e from both sides.

Remarks

  • As before, this feature is only effective when there is extra cloudflare.wrangler: {} config is provided
  • New mode is opt-in via EXPERIMENTAL_WRANGLER_CONFIG env variable
  • Old method is to be removed as soon as feat: add support for redirecting Wrangler to a generated config when running deploy commands cloudflare/workers-sdk#7442 is globally availble
  • We currently resolve configs from rootDir, it might need to be changed to cwd() later for monorepo support (pending more tests)
  • New support is added for reading wrangler.json from user
  • Merging strategy is to be improved, however we always respect explicit user config in wrangler.toml|json over

Local testing

  • pnpm build --stub
  • EXPERIMENTAL_WRANGLER_CONFIG=1 p nitro build playground --preset cloudflare_pages
ℹ Generated playground/dist/_routes.json                                                                                        
ℹ Generated playground/dist/_headers                                                                                             
ℹ Generated playground/dist/_redirects                                                                                           
ℹ Generated playground/.wrangler/deploy/config.json                                                                              
ℹ Generated playground/dist/_worker.js/wrangler.json 

.wrangler/deploy/config.json:

{"configPath":"../../dist/_worker.js/wrangler.json"}

wrangler.toml (user)

compatibility_flags = [ "nodejs_als" ]
name = "nitro-test"
compatibility_date = "2024-09-19"

[assets]
directory = "./.output/public/"
binding = "ASSETS"

[durable_objects]
[[durable_objects.bindings]]
name = "$DurableObject"
class_name = "$DurableObject"

[[migrations]]
tag = "v1"
new_classes = [ "$DurableObject" ]

dist/_worker.js/wrangler.json

{
  "compatibility_flags": [
    "nodejs_als",
    "nodejs_als"
  ],
  "name": "nitro-test",
  "compatibility_date": "2024-09-19",
  "assets": {
    "directory": "./.output/public/",
    "binding": "ASSETS"
  },
  "durable_objects": {
    "bindings": [
      {
        "name": "$DurableObject",
        "class_name": "$DurableObject"
      }
    ]
  },
  "migrations": [
    {
      "tag": "v1",
      "new_classes": [
        "$DurableObject"
      ]
    }
  ]
}

@petebacondarwin
Copy link

petebacondarwin commented Jan 8, 2025

Given this is using the cloudflare_pages plugin, this should not have an assets section in the user's wrangler.toml.
Nor should it have Durable Objects defined, since these are not valid in Pages projects.

This is likely to break the Pages CI.

Not a problem in real life because the user wouldn't have that. But in terms of testing, I will remove that from the wrangler.toml

@petebacondarwin
Copy link

For Pages projects that have a wrangler.toml it is required to have a pages_build_output_dir field in order that the config is considered for deployment. Otherwise it is completely ignored.

Depending on what you want the DX, you could either require the user to add that field themselves, or you could add it as part of the generation of wrangler.json. I think the latter is better, since I believe you want to generate a wrangler.json even if there is no user wrangler.toml.

Comment on lines +212 to +216
if (mergedConfig.pages_build_output_dir) {
throw new Error(
"`pages_build_output_dir` wrangler config should not be set."
);
}

Choose a reason for hiding this comment

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

Oh I see that you actively do not want the user to set this.
OK. But in that case you need to provide it automatically.

Choose a reason for hiding this comment

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

Perhaps add after the check?

    mergedConfig.pages_build_output_dir = nitro.options.output.publicDir;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants