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

Output .json by default in C3 #7676

Merged
merged 14 commits into from
Jan 10, 2025
Merged

Output .json by default in C3 #7676

merged 14 commits into from
Jan 10, 2025

Conversation

penalosa
Copy link
Contributor

@penalosa penalosa commented Jan 6, 2025

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):

for file in **/wrangler.toml; do       
    (toml2json "$file" | jq '{ name: .name, main: .main, compatibility_date: .compatibility_date } + .') > "${file%.toml}.json"
done

  • 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: C3 E2E will run without the label
  • Public documentation

@penalosa penalosa requested review from a team as code owners January 6, 2025 14:28
Copy link

changeset-bot bot commented Jan 6, 2025

🦋 Changeset detected

Latest commit: f1d26e5

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

Copy link
Contributor

@vicb vicb left a 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.

🙏

@penalosa
Copy link
Contributor Author

penalosa commented Jan 6, 2025

Generated files should look roughly like this:

// For more details on how to configure Wrangler, refer to:
// https://developers.cloudflare.com/workers/wrangler/configuration/
{
	"name": "test",
	"main": "src/index.ts",
	"compatibility_date": "2024-01-17",
	"$schema": "node_modules/wrangler/config-schema.json"
}

Does that seem like a sufficient level of commenting?

@vicb
Copy link
Contributor

vicb commented Jan 6, 2025

Generated files should look roughly like this:

// For more details on how to configure Wrangler, refer to:
// https://developers.cloudflare.com/workers/wrangler/configuration/
{
	"name": "test",
	"main": "src/index.ts",
	"compatibility_date": "2024-01-17",
	"$schema": "node_modules/wrangler/config-schema.json"
}

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.

Copy link
Contributor

github-actions bot commented Jan 6, 2025

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 with this latest build directly:

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.


[email protected] includes the following runtime dependencies:

Package Constraint Resolved
miniflare workspace:* 3.20241230.1
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.

@dario-piotrowicz
Copy link
Member

Nit:

Could you update this comment as well?

- * configuration value for wrangler.toml.
+ * configuration value for a wrangler config file

(or something like that)

Copy link
Member

@dario-piotrowicz dario-piotrowicz left a 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 🙂

@irvinebroque
Copy link
Contributor

irvinebroque commented Jan 7, 2025

Here is roughly what I think we should put in default config:

  • Logs
  • Smart Placement

^ 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.

@penalosa penalosa force-pushed the penalosa/c3-json-default branch from f48adac to 2677a9b Compare January 9, 2025 14:12
@vicb
Copy link
Contributor

vicb commented Jan 9, 2025

In the blocks

# Service Bindings (communicate between multiple Workers)
# https://developers.cloudflare.com/workers/wrangler/configuration/#service-bindings
# [[services]]
# binding = "MY_SERVICE"
# service = "my-service"

I think that the bottom half is kind of useless.

# [[services]]
# binding = "MY_SERVICE"
# service = "my-service"

I would rather trade it for 1 (one) line explaining what it enables.

@penalosa
Copy link
Contributor Author

penalosa commented Jan 9, 2025

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"
  // }]
}

@vicb vicb mentioned this pull request Jan 10, 2025
12 tasks
@irvinebroque
Copy link
Contributor

LGTM! (still getting used to JSONC)

@vicb
Copy link
Contributor

vicb commented Jan 10, 2025

LGTM! (still getting used to JSONC)

What do you think of swapping

  // "assets": {
  //   "directory": "./public/",
  //   "binding": "ASSETS"
  // },

for a description of what the binding (i.e. static assets here) does?

IMO that would be more helpful

@penalosa
Copy link
Contributor Author

@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

@vicb
Copy link
Contributor

vicb commented Jan 10, 2025

@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?
My point is that the templates are still huge with more useless inactive lines than actual config.

Anyway, this should not block this PR, we can tweak later if needed

Comment on lines 1 to 11
{
"name": "<TBD>",
"main": "./server.ts",
"compatibility_date": "<TBD>",
"assets": {
"directory": "./build/client"
},
"observability": {
"enabled": true
}
}
Copy link
Member

@edmundhung edmundhung Jan 10, 2025

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. 🤔

Copy link
Contributor Author

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added back in f1d26e5

Copy link
Contributor

@petebacondarwin petebacondarwin left a 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

@penalosa penalosa merged commit ab92f83 into main Jan 10, 2025
29 checks passed
@penalosa penalosa deleted the penalosa/c3-json-default branch January 10, 2025 19:04
penalosa added a commit that referenced this pull request Jan 10, 2025
* 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
CarmenPopoviciu added a commit that referenced this pull request Jan 15, 2025
[#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 added a commit that referenced this pull request Jan 15, 2025
[#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 added a commit that referenced this pull request Jan 15, 2025
[#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 added a commit that referenced this pull request Jan 15, 2025
[#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 added a commit that referenced this pull request Jan 15, 2025
[#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 added a commit that referenced this pull request Jan 15, 2025
[#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 added a commit that referenced this pull request Jan 15, 2025
[#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 added a commit that referenced this pull request Jan 16, 2025
* 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
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.

7 participants