-
Notifications
You must be signed in to change notification settings - Fork 760
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
Output .json
by default in C3
#7676
Conversation
🦋 Changeset detectedLatest commit: f1d26e5 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
packages/create-cloudflare/templates-experimental/astro/templates/js/wrangler.json
Outdated
Show resolved
Hide resolved
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.
I am 💯 to drop the clutter in the config file.
It might still be valuable to add a comment with a link to the binding config
What we discussed with @irvinebroque was to drop the 100+ lines of comments for a few lines with a link to the docs and check if it impacts binding creation (from the metrics we have).
My opinion is that the verbosity we had would only turn down users.
🙏
Generated files should look roughly like this:
Does that seem like a sufficient level of commenting? |
We should probably find a middle ground. I would be nice to list a few bindings that are either common or appealing to the users (KV, D1, R2, AI, ASSETS) but I would say no more than a couple lines. @irvinebroque might have some ideas. |
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/12714540652/npm-package-wrangler-7676 You can reference the automatically updated head of this PR with: npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/7676/npm-package-wrangler-7676 Or you can use npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12714540652/npm-package-wrangler-7676 dev path/to/script.js Additional artifacts:cloudflare-workers-bindings-extension: wget https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12714540652/npm-package-cloudflare-workers-bindings-extension-7676 -O ./cloudflare-workers-bindings-extension.0.0.0-v6c2b3f451.vsix && code --install-extension ./cloudflare-workers-bindings-extension.0.0.0-v6c2b3f451.vsix create-cloudflare: npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12714540652/npm-package-create-cloudflare-7676 --no-auto-update @cloudflare/kv-asset-handler: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12714540652/npm-package-cloudflare-kv-asset-handler-7676 miniflare: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12714540652/npm-package-miniflare-7676 @cloudflare/pages-shared: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12714540652/npm-package-cloudflare-pages-shared-7676 @cloudflare/unenv-preset: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12714540652/npm-package-cloudflare-unenv-preset-7676 @cloudflare/vitest-pool-workers: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12714540652/npm-package-cloudflare-vitest-pool-workers-7676 @cloudflare/workers-editor-shared: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12714540652/npm-package-cloudflare-workers-editor-shared-7676 @cloudflare/workers-shared: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12714540652/npm-package-cloudflare-workers-shared-7676 @cloudflare/workflows-shared: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12714540652/npm-package-cloudflare-workflows-shared-7676 Note that these links will no longer work once the GitHub Actions artifact expires.
Please ensure constraints are pinned, and |
...te-cloudflare/templates-experimental/hello-world-durable-object-with-assets/js/wrangler.json
Show resolved
Hide resolved
packages/create-cloudflare/templates-experimental/astro/templates/js/wrangler.toml
Show resolved
Hide resolved
Nit: Could you update this comment as well? - * configuration value for wrangler.toml.
+ * configuration value for a wrangler config file (or something like that) |
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.
Overall looks good to me 🙂
although I agree with @emily-shen that the ordering of fields seems less logical than before, I think that ideally we should update it (to make it as clear and intuitive as possible for users)
Also as I discussed with @vicb and @penalosa I am not really a fan of .json
files containing comments and would personally strongly prefer either getting rid of comments (and include them in the readme file for example) or use the .jsonc
extension instead. But if everyone else is happy with the .json
extension I'll live with it 🙂
packages/create-cloudflare/templates-experimental/next/templates/wrangler.json
Show resolved
Hide resolved
Here is roughly what I think we should put in default config:
^ these are foundational, and part of configuring your Worker Then explain/highlight bindings, with a link to a page that is a good jumping off point. Then, env vars (+callout about secrets, which is important), static assets and service bindings — bindings that are fundamental to Workers, often day zero things. Important to keep service bindings here and loudly discoverable. The natural thing to do otherwise is to just make http requests between Workers, get confused, etc. Below is TOML but JSONC would be equivalent name = "my-worker"
main = "src/index.js"
compatibility_date = "2024-12-30"
compatibility_flags = ["nodejs_compat"]
# Workers Logs
# https://developers.cloudflare.com/workers/observability/logs/workers-logs/
[observability]
enabled = true
# Smart Placement
# Docs: https://developers.cloudflare.com/workers/configuration/smart-placement/#smart-placement
# [placement]
# mode = "smart"
###
# Bindings
# Bindings allow your Worker to interact with resources on the Cloudflare Developer Platform, including
# databases, object storage, AI inference, real-time communication and more.
# https://developers.cloudflare.com/workers/runtime-apis/bindings/
###
# Environment Variables
# https://developers.cloudflare.com/workers/wrangler/configuration/#environment-variables
# [vars]
# MY_VARIABLE = "production_value"
# Note: Use secrets to store sensitive data.
# https://developers.cloudflare.com/workers/configuration/secrets/
# Static Assets
# https://developers.cloudflare.com/workers/static-assets/binding/
# [assets]
# directory = "./public/"
# binding = "ASSETS"
# Service Bindings (communicate between multiple Workers)
# https://developers.cloudflare.com/workers/wrangler/configuration/#service-bindings
# [[services]]
# binding = "MY_SERVICE"
# service = "my-service" @vicb @penalosa @nevikashah @korinne Think there's a way to get this simpler while still highlighting right things. |
f48adac
to
2677a9b
Compare
In the blocks
I think that the bottom half is kind of useless.
I would rather trade it for 1 (one) line explaining what it enables. |
How does this look @irvinebroque? (for the DO hello world template): /**
* For more details on how to configure Wrangler, refer to:
* https://developers.cloudflare.com/workers/wrangler/configuration/
*/
{
"$schema": "node_modules/wrangler/config-schema.json",
"compatibility_date": "2025-01-09",
"main": "src/index.ts",
"name": "tiny-grass-7ef3",
"migrations": [
{
"new_classes": [
"MyDurableObject"
],
"tag": "v1"
}
],
"durable_objects": {
"bindings": [
{
"class_name": "MyDurableObject",
"name": "MY_DURABLE_OBJECT"
}
]
},
"observability": {
"enabled": true
},
/**
* Smart Placement
* Docs: https://developers.cloudflare.com/workers/configuration/smart-placement/#smart-placement
*/
// "placement": { "mode": "smart" },
/**
* Bindings
* Bindings allow your Worker to interact with resources on the Cloudflare Developer Platform, including
* databases, object storage, AI inference, real-time communication and more.
* https://developers.cloudflare.com/workers/runtime-apis/bindings/
*/
/**
* Environment Variables
* https://developers.cloudflare.com/workers/wrangler/configuration/#environment-variables
*/
// "vars": {
// "MY_VARIABLE": "production_value"
// },
/**
* Note: Use secrets to store sensitive data.
* https://developers.cloudflare.com/workers/configuration/secrets/
*/
/**
* Static Assets
* https://developers.cloudflare.com/workers/static-assets/binding/
*/
// "assets": {
// "directory": "./public/",
// "binding": "ASSETS"
// },
/**
* Service Bindings (communicate between multiple Workers)
* https://developers.cloudflare.com/workers/wrangler/configuration/#service-bindings
*/
// "services": [{
// "binding": "MY_SERVICE",
// "service": "my-service"
// }]
} |
LGTM! (still getting used to JSONC) |
What do you think of swapping
for a description of what the binding (i.e. static assets here) does? IMO that would be more helpful |
@vicb IMO the example is more helpful—I could image just uncommenting the binding and starting to use it, wheread if this was just a description I'd still have to look up the syntax |
Can't the schema helps with with that? Anyway, this should not block this PR, we can tweak later if needed |
{ | ||
"name": "<TBD>", | ||
"main": "./server.ts", | ||
"compatibility_date": "<TBD>", | ||
"assets": { | ||
"directory": "./build/client" | ||
}, | ||
"observability": { | ||
"enabled": true | ||
} | ||
} |
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.
Will this remove the ability to update the name and compat date? FYI, the wrangler config from the remix template use custom build which works slightly different then what we are suggesting in c3. 🤔
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.
Oh interesting, is it not valid for C3? In that case I'll change this to TOML so it keeps overwriting the remix one, but we should probably fix that upstream?
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.
Added back in f1d26e5
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.
Briefly perused the changes and they LGTM
* Output .json by default * Create young-apes-battle.md * Address comments * Add more comments * Add more comments * sort json config files * Enable trailing commas * fix e2e * fix e2e again * formatting * Always install latest Wrangler * Don't overwrite the wrangler config that remix already provides * fix unit tests * Add back remix .toml file
* fix(create-cloudflare): Fix regression in C3's next template [#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 * further fixes of the regression PR * fix failing tests on windows
Fixes DEVX-1491
Generate all new projects from C3 with a
wrangler.json
file (cc @dario-piotrowicz), as well as simplifying the included comments (cc @vicb I know you had thoughts around this).I did the TOML -> JSON conversion by running (in
packages/create-cloudflare
):