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

Strip down wrangler.toml templates #6922

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Strip down wrangler.toml templates #6922

wants to merge 1 commit into from

Conversation

vicb
Copy link
Contributor

@vicb vicb commented Oct 8, 2024

What this PR solves / how to test

The wrangler.toml templates have 100+ lines of commented out template config.

I think it doesn't help users and potentially could make them think workers are more complex than they really are.

This is only a draft PR to get feedback.

What would be great is first to agree on the wording.

Then we should probably automatically inject the comments in all wrangler.toml instead of having to maintain 20 versions of those comments.

@irvinebroque @petebacondarwin thoughts?

Fixes N/A

Author has addressed the following

  • 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:
  • Changeset (Changeset guidelines)
    • TODO (before merge)
    • Changeset included
    • Changeset not necessary because:
  • Public documentation
    • TODO (before merge)
    • Cloudflare docs PR(s):
    • Documentation not necessary because:

@vicb vicb requested review from a team as code owners October 8, 2024 08:43
Copy link

changeset-bot bot commented Oct 8, 2024

⚠️ No Changeset found

Latest commit: 8acd557

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link
Contributor

github-actions bot commented Oct 8, 2024

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/11231813270/npm-package-wrangler-6922

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

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

Or you can use npx with this latest build directly:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11231813270/npm-package-wrangler-6922 dev path/to/script.js
Additional artifacts:
npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11231813270/npm-package-create-cloudflare-6922 --no-auto-update
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11231813270/npm-package-cloudflare-kv-asset-handler-6922
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11231813270/npm-package-miniflare-6922
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11231813270/npm-package-cloudflare-pages-shared-6922
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11231813270/npm-package-cloudflare-vitest-pool-workers-6922
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11231813270/npm-package-cloudflare-workers-editor-shared-6922
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11231813270/npm-package-cloudflare-workers-shared-6922

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


wrangler@3.80.1 includes the following runtime dependencies:

Package Constraint Resolved
miniflare workspace:* 3.20240925.1
workerd 1.20240925.0 1.20240925.0
workerd --version 1.20240925.0 2024-09-25

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

@irvinebroque
Copy link
Contributor

Goal of having this here is

  • discover what's possible
  • show how to configure fast w/examples

Can see how looks inelegant but would want to replace with something we believe genuinely more effective for onboarding and adoption of full platform, not just remove.

@nevikashah wdyt?

@vicb
Copy link
Contributor Author

vicb commented Oct 8, 2024

Noble goal.

My own experience is that this scared me the first time I generated a project.
Every time I start a new project from C3 deleting those lines is the first thing I do - it gives me a feeling of satisfaction!

For the records I have discussed with a few people and they were +1 to strip this

@petebacondarwin
Copy link
Contributor

petebacondarwin commented Oct 8, 2024

I am also put off by this large commented block; moreover it is a pain to maintain since every time someone adds a binding we have to go and update all these templates.

How about we just have a single comment that links to a web page in the docs that shows a fully featured example wrangler.toml wired up to lots of bindings?

@vicb
Copy link
Contributor Author

vicb commented Oct 8, 2024

How about we just have a single comment that links to a web page in the docs that shows a fully featured example wrangler.toml wired up to lots of bindings?

Wouldn't "a web page in the docs that shows a fully featured example wrangler.toml wired up to lots of bindings" move the exact same problem to a different place?

IIUC the initial intent was to help new comers. Can't we have an example with a few bindings (do we have stats on the most common? probably assets, KV, plus a couple others) and link to that.

For more advanced users we can link the (reference) docs and they'll be able to find what they want there.

@petebacondarwin
Copy link
Contributor

Wouldn't "a web page in the docs that shows a fully featured example wrangler.toml wired up to lots of bindings" move the exact same problem to a different place?

I believe it would be better for two reasons:

  1. There would only be one page to update, rather than many templates. I note you only changed the analog one in this PR but there are actually loads and loads of these wrangler.tomls
  2. It would still provide the user with just one click to get access to the information we are trying to make accessible by having this big comment in the template

@vicb
Copy link
Contributor Author

vicb commented Oct 8, 2024

  1. There would only be one page to update, rather than many templates. I note you only changed the analog one in this PR but there are actually loads and loads of these wrangler.tomls

I mentioned that in the PR description:

Then we should probably automatically inject the comments in all wrangler.toml instead of having to maintain 20 versions of those comments.

(That still holds if we move to a more reasonably sized comment)

@vicb
Copy link
Contributor Author

vicb commented Oct 14, 2024

@irvinebroque @nevikashah Can we please move forward here?

I really think we should keep things simple and not overwhelm fist time users with hundreds of lines of commented out configuration lines - in the same spirit, I've proposed #6957.

Providing a short text and a link to the documentation would be as efficient without making workers look more complicated than they really are.

Thanks!

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

Successfully merging this pull request may close these issues.

3 participants