widen multi-env `vars` types in `wrangler types` and add `--strict-vars` option to the command #5086

dario-piotrowicz · 2024-02-24T17:29:44Z

Fixes #5082

This PR does two things:

one is widening the vars types that wrangler types generates when the same variables are defined in different environments (basically instead of incorrectly generate something like MY_VAR: "dev value"; now we could correctly generate MY_VAR: "dev value" | "prod value"; etc...)
two is adding a new option (currently called --strict-vars) that allows the generated types not to be literals/unions but to be loose/generic ones (e.g. instead of MY_VAR: "dev value" | "prod value"; it allows the generation of MY_VAR: string;), this was added after various feedback and back and forth from the team (the concern being that literal values/unions can be annoying for people if they change their var values often)

For more details please check the changeset files in the PR 🙂

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: this is tested in unit tests
Public documentation
- TODO (before merge)
- Cloudflare docs PR(s): add --strict-vars option to the wrangler types command cloudflare-docs#18981
- Documentation not necessary because:

changeset-bot · 2024-02-24T17:29:47Z

🦋 Changeset detected

Latest commit: 5405cd8

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

This PR includes changesets to release 2 packages

Name	Type
wrangler	Minor
@cloudflare/vitest-pool-workers	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 · 2024-02-24T17:32:02Z

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/12679174554/npm-package-wrangler-5086

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

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

Or you can use npx with this latest build directly:

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

Additional artifacts:

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

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

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

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

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

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

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

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

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

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

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

wrangler@3.100.0 includes the following runtime dependencies:

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

codecov · 2024-02-24T17:35:37Z

Codecov Report

Attention: Patch coverage is 97.36842% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 70.42%. Comparing base (cab7e1c) to head (df55b66).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5086      +/-   ##
==========================================
+ Coverage   70.33%   70.42%   +0.08%     
==========================================
  Files         298      298              
  Lines       15515    15536      +21     
  Branches     3987     3990       +3     
==========================================
+ Hits        10913    10941      +28     
+ Misses       4602     4595       -7

Files	Coverage Δ
packages/wrangler/src/type-generation.ts	`99.29% <100.00%> (+0.09%)`	⬆️
packages/wrangler/src/config/index.ts	`90.44% <91.66%> (+0.27%)`	⬆️

... and 6 files with indirect coverage changes

dario-piotrowicz · 2024-02-26T17:54:49Z

I've converted this PR to draft as it's been decided by the team that we should go with generic string, etc... types instead of union of string literals etc...

DaniFoldi · 2024-02-26T18:45:32Z

Hi 👋,

Thanks @dario-piotrowicz for the PR, this is something we had to fix manually after running wrangler types. I believe your implementation would improve correctness, and I was looking forward to seeing it land. Widening the type to string is a weird change in my opinion - updating a vars variable is no different to adding/deleting a KV namespace, it causes the same desync between the config and the type declaration file. Some frameworks auto-generate their equivalent of wrangler types (eg. astro sync, nuxi prepare) before building or starting a dev server, although I admit that's a slightly different case as those files would generally be in .gitignore. Still, I don't think a union would cause any more harm than string.

jasperdunn · 2024-07-21T06:21:57Z

I'd be keen to know what the reasoning was for using a generic string type instead of string literals. Surely having more robust types would benefit those of us using typescript? My solution atm is to override the type.

jfsiii · 2024-10-31T13:39:06Z

I've converted this PR to draft as it's been decided by the team that we should go with generic string, etc... types instead of union of string literals etc...

@dario-piotrowicz Is there a place to track that decision/implementation?

I'd also prefer a union but I'm mostly looking to avoid this bug

dario-piotrowicz · 2024-10-31T13:47:56Z

I've converted this PR to draft as it's been decided by the team that we should go with generic string, etc... types instead of union of string literals etc...

@dario-piotrowicz Is there a place to track that decision/implementation?

I'd also prefer a union but I'm mostly looking to avoid this bug

Hi @jfsiii 👋

Sorry, honestly this has fallen off my radar and I haven't pushed for a resolution, it's been quite a long time since this PR has been opened... let me ping the team and prompt them to make a decision, then I can update and merge this PR 🙂👍

kansson · 2024-12-29T15:45:55Z

I've converted this PR to draft as it's been decided by the team that we should go with generic string, etc... types instead of union of string literals etc...

@dario-piotrowicz Is there a place to track that decision/implementation?
I'd also prefer a union but I'm mostly looking to avoid this bug

Hi @jfsiii 👋

Sorry, honestly this has fallen off my radar and I haven't pushed for a resolution, it's been quite a long time since this PR has been opened... let me ping the team and prompt them to make a decision, then I can update and merge this PR 🙂👍

Hey just checking in on the progress. Thanks 😄

dario-piotrowicz · 2025-01-01T15:52:41Z

Hey just checking in on the progress. Thanks 😄

@kansson thanks for the ping 🙂 , I checked with the team a few days ago and it was suggested that I add a flag to allow having the loosen/generic types alongside the literal/union ones, so that we can have both the stricter/robust types alongside loosen/generic ones if needed (to try to satisfy everyone)

That's what I did with the new --loose-vars flag (the name can still change).

I've also rebased the PR, it's ready to be re-reviewer and hopefully merged soon 🙂

@jfsiii so sorry for the late action here 😓 (not to mention @DaniFoldi 😅 🙇)

packages/wrangler/src/type-generation/index.ts

dario-piotrowicz · 2025-01-01T16:39:23Z

packages/wrangler/src/__tests__/type-generation.test.ts

@@ -603,6 +603,35 @@ describe("generateTypes()", () => {
 	`);
 	});

+	it("should respect the loose-vars option", async () => {


I was considering adding a test for vars generation without loose-vars, but since that's the default behavior (and given the "vars present in multiple environments" describe block) that felt a bit overkill here 😕 I'm happy to do it if someone thinks that that would be good

actually given the "vars present in multiple environments" describe block, this test might be a bit overkill... I'm also happy to remove it entirely 🙂

dario-piotrowicz · 2025-01-01T16:43:02Z

@petebacondarwin, @andyjessop please have a look when you get the chance 🙏

penalosa

In general, could you add some more comments explaining the logic? There's some fairly complicated code here

.changeset/unlucky-timers-sort.md

packages/wrangler/src/type-generation/index.ts

dario-piotrowicz · 2025-01-07T14:45:35Z

In general, could you add some more comments explaining the logic? There's some fairly complicated code here

All the types gen logic here seems to me fairly complicated but without any comments, so I was just trying to adhere to the code style in these files, if code consistency is not a concern I'll add some comments to the new logic 👍

Amend: 👆 sorry I misremembered, that's not actually true, there are comments in some places but not in others

.changeset/proud-forks-build.md

.changeset/unlucky-timers-sort.md

dario-piotrowicz · 2025-01-07T18:26:32Z

@penalosa I've simplified the logic I had (alongside the var values I was also needlessly collecting their environment name, I did so as it might come in handy in the future, but I think that it's just better to keep things simple and add it later if the extra complexity becomes necessary) and I added a few comments, hopefully the code should be simpler to follow now, please have another look and let me know 🙏

dario-piotrowicz · 2025-01-07T18:29:11Z

packages/wrangler/src/type-generation/index.ts

@@ -262,11 +247,14 @@ async function generateTypes(
 		);
 	}

-	const envTypeStructure: string[] = [];
+	const envTypeStructure: [string, string][] = [];


I think it would be nicer/clearer to have this as

const envTypeStructure: { key: string, type: string, }[] = [];

but it would make all the .push calls below much more verbose, and the current code should be clear enough

but I am happy with either solution, if anyone prefers the more verbose version (or has any other idea) I'm happy to change this

dario-piotrowicz · 2025-01-07T18:30:24Z

@petebacondarwin sorry I had to change the code significantly after your approval 😓 🙇 , if you would please have another look (PS: anyways the functionality didn't change, I just cleaned up the code and fixes something I broke)

packages/wrangler/src/__tests__/type-generation.test.ts

rename `loose-vars` to `strict-vars`

Co-authored-by: Somhairle MacLeòid <smacleod@cloudflare.com>

Co-authored-by: Pete Bacon Darwin <pete@bacondarwin.com>

dario-piotrowicz requested a review from a team as a code owner February 24, 2024 17:29

dario-piotrowicz mentioned this pull request Feb 24, 2024

🚀 Feature Request: wrangler types generates wider types for environment variables #5082

Open

dario-piotrowicz marked this pull request as draft February 26, 2024 17:53

stevezhu mentioned this pull request Jul 21, 2024

🚀 Feature Request: Generate more general types for wrangler types #6303

Closed

andyjessop mentioned this pull request Nov 25, 2024

🐛 BUG: wrangler types copies actual string from wrangler.toml instead of the type #7273

Closed

dario-piotrowicz force-pushed the wrangler-types-string-literals branch from df55b66 to b4ac0d9 Compare January 1, 2025 15:41

dario-piotrowicz added skip-pr-description-validation Skip validation of the required PR description format and removed skip-pr-description-validation Skip validation of the required PR description format labels Jan 1, 2025

dario-piotrowicz mentioned this pull request Jan 1, 2025

add --strict-vars option to the wrangler types command cloudflare/cloudflare-docs#18981

Draft

1 task

dario-piotrowicz changed the title ~~fix: widen multi-env vars types in wrangler types~~ widen multi-env vars types in wrangler types and add --loose-vars option to the command Jan 1, 2025

dario-piotrowicz commented Jan 1, 2025

View reviewed changes

dario-piotrowicz marked this pull request as ready for review January 1, 2025 16:42

dario-piotrowicz requested review from petebacondarwin and andyjessop January 1, 2025 16:42

dario-piotrowicz changed the title ~~widen multi-env vars types in wrangler types and add --loose-vars option to the command~~ widen multi-env vars types in wrangler types and add --strict-vars option to the command Jan 2, 2025

penalosa reviewed Jan 7, 2025

View reviewed changes

.changeset/unlucky-timers-sort.md Outdated Show resolved Hide resolved

packages/wrangler/src/type-generation/index.ts Outdated Show resolved Hide resolved

packages/wrangler/src/type-generation/index.ts Outdated Show resolved Hide resolved

petebacondarwin approved these changes Jan 7, 2025

View reviewed changes

.changeset/proud-forks-build.md Outdated Show resolved Hide resolved

.changeset/proud-forks-build.md Outdated Show resolved Hide resolved

.changeset/unlucky-timers-sort.md Outdated Show resolved Hide resolved

.changeset/unlucky-timers-sort.md Outdated Show resolved Hide resolved

dario-piotrowicz requested a review from penalosa January 7, 2025 18:26

dario-piotrowicz commented Jan 7, 2025

View reviewed changes

dario-piotrowicz requested a review from petebacondarwin January 7, 2025 18:29

petebacondarwin approved these changes Jan 8, 2025

View reviewed changes

packages/wrangler/src/__tests__/type-generation.test.ts Show resolved Hide resolved

packages/wrangler/src/__tests__/type-generation.test.ts Show resolved Hide resolved

dario-piotrowicz force-pushed the wrangler-types-string-literals branch from e366aed to e4d2892 Compare January 8, 2025 16:16

dario-piotrowicz and others added 12 commits January 8, 2025 22:16

fix: widen multi-env vars types in wrangler types

6fbfa01

add loose-vars option to wrangler types

10a8797

fixup! add loose-vars option to wrangler types

ada4e7d

rename `loose-vars` to `strict-vars`

Update .changeset/unlucky-timers-sort.md

c262fc5

Co-authored-by: Somhairle MacLeòid <smacleod@cloudflare.com>

remove unnecessary double map

2b1fd3e

fix improve and simplify types generation logic

52648c6

Update .changeset/proud-forks-build.md

7e57589

Co-authored-by: Pete Bacon Darwin <pete@bacondarwin.com>

Update .changeset/unlucky-timers-sort.md

1d8fb94

Co-authored-by: Pete Bacon Darwin <pete@bacondarwin.com>

remove unnecessary line

bf39a65

clarify --strict-vars changeset

8a6d0dd

simplify logic and add comments

41e01a6

update the to a

5405cd8

dario-piotrowicz force-pushed the wrangler-types-string-literals branch from e4d2892 to 5405cd8 Compare January 8, 2025 21:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

widen multi-env `vars` types in `wrangler types` and add `--strict-vars` option to the command #5086

widen multi-env `vars` types in `wrangler types` and add `--strict-vars` option to the command #5086

dario-piotrowicz commented Feb 24, 2024 •

edited

Loading

changeset-bot bot commented Feb 24, 2024 •

edited

Loading

github-actions bot commented Feb 24, 2024 •

edited

Loading

codecov bot commented Feb 24, 2024 •

edited

Loading

dario-piotrowicz commented Feb 26, 2024

DaniFoldi commented Feb 26, 2024

jasperdunn commented Jul 21, 2024

jfsiii commented Oct 31, 2024

dario-piotrowicz commented Oct 31, 2024

kansson commented Dec 29, 2024

dario-piotrowicz commented Jan 1, 2025

dario-piotrowicz Jan 1, 2025

dario-piotrowicz Jan 1, 2025

dario-piotrowicz commented Jan 1, 2025

penalosa left a comment

dario-piotrowicz commented Jan 7, 2025 •

edited

Loading

dario-piotrowicz commented Jan 7, 2025

dario-piotrowicz Jan 7, 2025

dario-piotrowicz commented Jan 7, 2025

widen multi-env vars types in wrangler types and add --strict-vars option to the command #5086

Are you sure you want to change the base?

widen multi-env vars types in wrangler types and add --strict-vars option to the command #5086

Conversation

dario-piotrowicz commented Feb 24, 2024 • edited Loading

changeset-bot bot commented Feb 24, 2024 • edited Loading

🦋 Changeset detected

github-actions bot commented Feb 24, 2024 • edited Loading

codecov bot commented Feb 24, 2024 • edited Loading

Codecov Report

dario-piotrowicz commented Feb 26, 2024

DaniFoldi commented Feb 26, 2024

jasperdunn commented Jul 21, 2024

jfsiii commented Oct 31, 2024

dario-piotrowicz commented Oct 31, 2024

kansson commented Dec 29, 2024

dario-piotrowicz commented Jan 1, 2025

dario-piotrowicz Jan 1, 2025

Choose a reason for hiding this comment

dario-piotrowicz Jan 1, 2025

Choose a reason for hiding this comment

dario-piotrowicz commented Jan 1, 2025

penalosa left a comment

Choose a reason for hiding this comment

dario-piotrowicz commented Jan 7, 2025 • edited Loading

dario-piotrowicz commented Jan 7, 2025

dario-piotrowicz Jan 7, 2025

Choose a reason for hiding this comment

dario-piotrowicz commented Jan 7, 2025

widen multi-env `vars` types in `wrangler types` and add `--strict-vars` option to the command #5086

widen multi-env `vars` types in `wrangler types` and add `--strict-vars` option to the command #5086

dario-piotrowicz commented Feb 24, 2024 •

edited

Loading

changeset-bot bot commented Feb 24, 2024 •

edited

Loading

github-actions bot commented Feb 24, 2024 •

edited

Loading

codecov bot commented Feb 24, 2024 •

edited

Loading

dario-piotrowicz commented Jan 7, 2025 •

edited

Loading