fix(create-cloudflare): Fix regression in C3's next template by CarmenPopoviciu · Pull Request #7774 · cloudflare/workers-sdk

CarmenPopoviciu · 2025-01-15T11:31:42Z

#7676 switched C3 templates to default to wrangler.json instead of wrangler.toml. Unfortunately, this unintendedly broke the next template, which was still attempting to read wrangler.toml specifically.

This commit fixes the regression.

Fixes #7770

These changes were also tested using a test project and the C3 pre-release url . See snapshot of generated readme file 👇

Tests
- TODO (before merge)
- Tests included
- Tests not necessary because:
E2E Tests CI Job required? (Use "e2e" label or ask maintainer to run separately)
- I don't know
- Required
- Not required because:
Public documentation
- TODO (before merge)
- Cloudflare docs PR(s):
- Documentation not necessary because: regression fix

changeset-bot · 2025-01-15T11:31:46Z

🦋 Changeset detected

Latest commit: bea9a39

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
create-cloudflare	Patch

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 · 2025-01-15T11:34:54Z

A wrangler prerelease is available for testing. You can install this latest build in your project with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12798470841/npm-package-wrangler-7774

You can reference the automatically updated head of this PR with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/7774/npm-package-wrangler-7774

Or you can use npx with this latest build directly:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12798470841/npm-package-wrangler-7774 dev path/to/script.js

Additional artifacts:

cloudflare-workers-bindings-extension:

wget https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12798470841/npm-package-cloudflare-workers-bindings-extension-7774 -O ./cloudflare-workers-bindings-extension.0.0.0-v9f74e3def.vsix && code --install-extension ./cloudflare-workers-bindings-extension.0.0.0-v9f74e3def.vsix

create-cloudflare:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12798470841/npm-package-create-cloudflare-7774 --no-auto-update

@cloudflare/kv-asset-handler:

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12798470841/npm-package-cloudflare-kv-asset-handler-7774

miniflare:

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12798470841/npm-package-miniflare-7774

@cloudflare/pages-shared:

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12798470841/npm-package-cloudflare-pages-shared-7774

@cloudflare/unenv-preset:

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12798470841/npm-package-cloudflare-unenv-preset-7774

@cloudflare/vitest-pool-workers:

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12798470841/npm-package-cloudflare-vitest-pool-workers-7774

@cloudflare/workers-editor-shared:

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12798470841/npm-package-cloudflare-workers-editor-shared-7774

@cloudflare/workers-shared:

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12798470841/npm-package-cloudflare-workers-shared-7774

@cloudflare/workflows-shared:

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12798470841/npm-package-cloudflare-workflows-shared-7774

Note that these links will no longer work once the GitHub Actions artifact expires.

wrangler@3.102.0 includes the following runtime dependencies:

Package	Constraint	Resolved
`miniflare`	workspace:*	3.20241230.2
`workerd`	1.20241230.0	1.20241230.0
`workerd --version`	1.20241230.0	2024-12-30

Please ensure constraints are pinned, and miniflare/workerd minor versions match.

CarmenPopoviciu · 2025-01-15T13:50:37Z

we should start adding comments when we quarantine tests

we should also evaluate why tests still need to be quarantined (maybe an investigation + adding respective comments) would be a good start 🙂

PS: as far as I can tell quarantined tests get forgotten and their templates can completely break without anyone noticing, I wonder if we could increase the visibility of these too

(that's actually what happened here isn't it? 🤔 next broke but no one noticed right away since it's quarantined)

that is correct yes. and I agree that we should evaluate what tests are quarantined and why. I took a quick look and both astro and analog tests are quarantined. I have context over the analog ones but not the astro ones

CarmenPopoviciu · 2025-01-15T14:58:31Z

to keep things consistent with the rest of the comments in the config file, I added the KV comment in a similar format. IMHO, it made no sense to add the extra // KV example line, so I didn't, and modified the readme instead. Lemme know if anyone strongly disagrees

I don't know if adding the kv_namespaces only for next makes sense... (why KV is the only binding included? what about DOs, R2, etc... why the preferential treatment?) as we discussed maybe we should just get rid of the KV example in the next template instead 🤔

I'm happy to progress with this solution but (maybe as a followup) I think removing the example would be better

why KV is the only binding included? what about DOs, R2, etc... why the preferential treatment?

I totally get what you mean. But then, wasn't this question equally relevant in the prev version? Why did the nextjs starter ship only with a KV example? I have no context whatsoever as to why/how this decision was taken.

Don't get me wrong, I don't mind removing it, but then my question is, does C3 come with anything similar out of the box?...as in an example or template that exemplify how such bindings are set. Does it need to? Arguably, these are all larger prod questions

I totally get what you mean. But then, wasn't this question equally relevant in the prev version?

yeah I agree with you, this was basically the same before, the Next.js template being the only framework one having an extra KV example 👍

I'm just bringing it up because before it was less expensive to have this divergence (basically a single extra comment in the shared toml file) while now it seems like it requires us to add the KV declaration comment to all wrangler configs, given this extra downside maybe it's no longer worth it

Why did the nextjs starter ship only with a KV example?

We discussed it on the frameworks team and concluded that it'd be nice to show such an example to make sure that people knew how to access bindings in their Next.js app, that's all

does C3 come with anything similar out of the box?

I'd have to check, I am quite sure the Next.js one is the only fullstack template with this, as for workers I would imagine that they also have extra configs as well

Don't get me wrong, I don't mind removing it

I don't particularly mind if you keep it in, I just personally think that the extra (even if very small) complexity + having the KV for all template configs makes the example not that KV Next.js example worth keeping, but it's also not a big deal 🙂

while now it seems like it requires us to add the KV declaration comment to all wrangler configs

I am 100% with you on this! And tbh, I do have the same concern as you. On the other hand, I 100% believe that the KV example is valuable for our users, which is why I'm holding on to it with my dear life 😆 . IMHO if it's not meant to live in the starter project it should def live somewhere else...tho the starter is such a great place, because it requires zero context switch on the dev's side

We discussed it on the frameworks team and concluded that it'd be nice to show such an example to make sure that people knew how to access bindings in their Next.js app, that's all

yes 💯 this!!!!

having said all this I feel like there is not enough consensus (wrt what the right fix is) to land this PR just yet. How about if I just revert the next template back to wrangler.toml. The remix template is still using a .toml config file, so it wouldn't be the only exception. Just so we can bump the C3 version with a fix. And in the meanwhile, we can iterate this PR without the pressure of a fix release on our back

oh wait...we actually can't revert back to wrangler.toml because of how we now standardise comments. NVM me

Nono, I am totally happy to go with the current solution! we can always re-evaluate later! no big deal 🙂

emily-shen

Not blocking this PR, but there's also a bunch of other places that use wrangler.toml, I think next is just more commonly use so it's come up here.
I think wrangler.toml is still used in

hello world + hello world with assets vitest config
download pre-existing worker from dash
comment on the worker-configuration.d.ts files

sorry should've caught these while reviewing the original PR!

dario-piotrowicz · 2025-01-15T17:09:37Z

It's pretty minor and could be done in a followup PR, but if you follow the readme instructions:

workers-sdk/packages/create-cloudflare/templates/next/README.md

Lines 40 to 64 in 99f27df

    
           #### KV Example 
        
           `c3` has added for you an example showing how you can use a KV binding. 
        
           In order to enable the example: 
        
           - Search for javascript/typescript lines containing the following comment: 
        
             ```ts 
        
             // KV Example: 
        
             ``` 
        
             and uncomment the commented lines below it (also uncomment the relevant imports). 
        
           - Do the same in the `wrangler.toml` file, where 
        
             the comment is: 
        
             ``` 
        
             # KV Example: 
        
             ``` 
        
           - If you're using TypeScript run the `cf-typegen` script to update the `env.d.ts` file: 
        
             ```bash 
        
             npm run cf-typegen 
        
             # or 
        
             yarn cf-typegen 
        
             # or 
        
             pnpm cf-typegen 
        
             # or 
        
             bun cf-typegen 
        
             ```

things don't really work well as a comma is missing in the wrangler.json 😓

People should not get too confused by this but to fix it we should add a comma here (I think?):

workers-sdk/packages/create-cloudflare/src/wrangler/config.ts

Line 57 in 99f27df

stringified.slice(0, -2) +

(or just mention in the readme to add the comma?)

dario-piotrowicz

Looks good to me, thanks @CarmenPopoviciu 😄

[#7676](#7676) switched C3 templates to default to `wrangler.json` instead of `wrangler.toml`. Unfortunately, this unintendedly broke the `next` template, which was still attempting to read `wrangler.toml` specifically. This commit fixes the regression. Fixes #7770

CarmenPopoviciu · 2025-01-16T09:33:42Z

just for posterity...

fix(create-cloudflare): Fix regression in C3's next template #7774 (comment)

there was some back forth in this conversation, rightfully so, about whether this PR fixes things in a way the we want things to be moving forward. Landing the PR as it was at the time of this conversation, would also have left a couple of loose ends that would have had to be addressed in follow-up PRs. Therefore we pulled in prod, and asked them to take a decision, since this felt very much like a prod decision to take. The solution we landed, which is reflected in the current state of this PR, was to keep the next template as an exception, in terms of providing that KV example, but instead of the template relying on particular commented code being present in the config file, we instead changed the readme to instruct users to add the necessary configuration to their config file. This means that we ultimately didn't have to pollute the config file for all C3 templates, with comments about KV bindings and such.

fix(create-cloudflare): Fix regression in C3's next template #7774 (comment)

because of the adopted solution, this is no longer applicable

fix(create-cloudflare): Fix regression in C3's next template #7774 (review)

we went ahead and solved this as well. In theory, there should be no lingering references of wrangler.toml in the C3 templates, with the exception of Remix (the reason Remix is still on wrangler.toml is that we start from the Remix cf Workers template, which provides its own wrangler.toml (and so adding in a wrangler.json meant ending up with two config files) was not yet ported to wrangler.json

andyjessop

Really nice investigation and PR. Thanks so much for this 🚀

CarmenPopoviciu requested a review from a team January 15, 2025 11:31

CarmenPopoviciu requested a review from a team as a code owner January 15, 2025 11:31

CarmenPopoviciu added the e2e Run wrangler + vite-plugin e2e tests on a PR label Jan 15, 2025

CarmenPopoviciu force-pushed the carmen/fix-nextjs-template branch 4 times, most recently from 50b3f6e to 9cc12d5 Compare January 15, 2025 13:48

CarmenPopoviciu commented Jan 15, 2025

View reviewed changes

CarmenPopoviciu force-pushed the carmen/fix-nextjs-template branch 2 times, most recently from eee742f to e42878f Compare January 15, 2025 14:52

CarmenPopoviciu commented Jan 15, 2025

View reviewed changes

emily-shen approved these changes Jan 15, 2025

View reviewed changes

dario-piotrowicz reviewed Jan 15, 2025

View reviewed changes

Comment thread packages/create-cloudflare/templates/next/README.md Outdated

penalosa approved these changes Jan 15, 2025

View reviewed changes

dario-piotrowicz reviewed Jan 15, 2025

View reviewed changes

Comment thread packages/create-cloudflare/src/wrangler/__tests__/config.test.ts Outdated

CarmenPopoviciu force-pushed the carmen/fix-nextjs-template branch from 0575c03 to 92feb25 Compare January 15, 2025 15:45

This comment was marked as off-topic.

Sign in to view

dario-piotrowicz approved these changes Jan 15, 2025

View reviewed changes

CarmenPopoviciu force-pushed the carmen/fix-nextjs-template branch from 92feb25 to 76200e9 Compare January 15, 2025 18:27

CarmenPopoviciu force-pushed the carmen/fix-nextjs-template branch from 76200e9 to ac0b555 Compare January 15, 2025 22:12

CarmenPopoviciu added 2 commits January 15, 2025 23:30

further fixes of the regression PR

0684171

fix failing tests on windows

bea9a39

This was referenced Jan 16, 2025

🐛 BUG: create-cloudflare@2.37.0, Output the wrangler.json config format when running create-cloudflare. But when generating the next template, the wrangler.toml file will be read. #7770

Closed

Version Packages #7766

Merged

andyjessop approved these changes Jan 16, 2025

View reviewed changes

CarmenPopoviciu merged commit f9344a0 into main Jan 16, 2025

CarmenPopoviciu deleted the carmen/fix-nextjs-template branch January 16, 2025 09:40

irvinebroque mentioned this pull request Jan 25, 2025

🐛 BUG: New Hono project from C3 creates wrangler.toml and wrangler.json #7905

Closed

Conversation

CarmenPopoviciu commented Jan 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

changeset-bot Bot commented Jan 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

github-actions Bot commented Jan 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CarmenPopoviciu Jan 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emily-shen left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dario-piotrowicz commented Jan 15, 2025

Uh oh!

This comment was marked as off-topic.

dario-piotrowicz left a comment

Choose a reason for hiding this comment

Uh oh!

CarmenPopoviciu commented Jan 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andyjessop left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

CarmenPopoviciu commented Jan 15, 2025 •

edited

Loading

changeset-bot Bot commented Jan 15, 2025 •

edited

Loading

github-actions Bot commented Jan 15, 2025 •

edited

Loading

CarmenPopoviciu Jan 15, 2025 •

edited

Loading

emily-shen left a comment •

edited

Loading

CarmenPopoviciu commented Jan 16, 2025 •

edited

Loading