fix(wrangler): prevent invalid JS identifiers in types generation

[Cherry]

Fixes #5090

What this PR solves / how to test:

Previously, dashes in bindings or vars would result in an invalid output with wrangler types. With the following:

[vars]
some-var = "foobar"

You would get:

interface Env {
	some-var: "foobar";
}

which is syntactically invalid. This PR fixes that by wrapping necessary keys in quotes. So now you get:

interface Env {
	"some-var": "foobar";
}

Author has addressed the following:

• Tests
  • Included
  • Not necessary because:
• Changeset (Changeset guidelines)
  • Included
  • Not necessary because:
• Associated docs
  • Issue(s)/PR(s):
  • Not necessary because:

Note for PR author:

We want to celebrate and highlight awesome PR review! If you think this PR received a particularly high-caliber review, please assign it the label highlight pr review so future reviewers can take inspiration and learn from it.

[changeset-bot]

🦋 Changeset detected

Latest commit: 98d6048

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

This PR includes changesets to release 2 packages

Name	Type
wrangler	Patch
@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]

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/8561737111/npm-package-wrangler-5091

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

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

Or you can use npx with this latest build directly:

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

Additional artifacts:

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

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

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

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

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

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

---

wrangler@3.47.0 includes the following runtime dependencies:

Package	Constraint	Resolved
miniflare	workspace:*	3.20240403.0
workerd	1.20240403.0	1.20240403.0
workerd --version	1.20240403.0	2024-04-03

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

[dario-piotrowicz]

@Cherry awesome stuff! the missing dashes handling is definitely something that's been overlooked, thanks a lot for looking into fixing that 😄

But two things, one I think that you're not updating all the tests.

For example:

Regardless, I would personally not go with a solution in which we quote all possible keys but only apply quotes when needed (i.e. when a field name/binding does contain a dash), doing it only when necessary seems to me like a much nicer/more polished behavior

what do you think?

[Cherry]

Thanks @dario-piotrowicz. I did miss some tests, good catch!

I agree it would be good to only quote when needed. I originally opted for consistency, since that's what I personally use in my ESLint config (https://eslint.style/rules/default/quote-props consistent), but that's definitely opinionated.

On further thought and discussion though, I think there's significantly more edge-cases here that end up syntactically invalid. While not all valid binding names, these are all valid vars:

[vars]
dashed-var="foo"
"single-quote'"="bar"
'"double-quote"'="baz"
"escaped\"quote"="qux"
"escaped\\backslash"="quux"
123num="quuz'"
123numquote="'''"
true=1
false=42
null=0

Which today results in the following:

interface Env {
	dashed-var: "foo";
	single-quote': "bar";
	"double-quote": "baz";
	escaped"quote: "qux";
	escaped\backslash: "quux";
	123num: "quuz'";
	123numquote: "'''";
	true: "1";
	false: "42";
	null: "0";
}

With my PR here, it's slightly better, but still not great:

interface Env {
	"dashed-var": "foo";
	"single-quote'": "bar";
	""double-quote"": "baz";
	"escaped"quote": "qux";
	"escaped\backslash": "quux";
	"123num": "quuz'";
	"123numquote": "'''";
	"true": "1";
	"false": "42";
	"null": "0";
}

My PR handles the vars starting with numbers, the dashes, and the weird true, false, null etc. literals but falls over once you introduce quotes and escaping other chars. This is likely going to need some better thought and handling. I'm happy to take another look later this week, but if you have an idea for how you want to best support this, please feel free.

The current implementation also seems to assume that variables are always strings, but when you do foo=1 in vars, it's available under env as a number, and not a string. Example: https://test-name.jross.workers.dev/, which is just return Response.json(env) using the example above.

Many of these are edge-cases and probably not real-world, but I think dashes, and starting with numbers are very valid cases to be covered. You can do fun stuff like "breaking /*" = "testing" with the current implementation and break the rest of the generation too (very edge-casey).

[dario-piotrowicz]

@Cherry Regarding the optional inclusion of quotes, I think that it could be great to add the quotes only when needed and maybe have an option like: --with-consistency or something along those lines if you always want quotes...?

Although maybe I would not even add the flag and just have the quotes only added when needed. Users can very easily then just use their formatter to adjust the file after the fact anyways right? they'll get their style as soon as they run prettier or whatever they use anyways no?

Mh... based on my argument above you could actually argue that you could always add quotes for simplicity and developers can remove them with their formatter right? 😅 🤷 So I am not too too sure 😅 (but I am opinionated on having it only when needed 😛)

---

Regarding edge cases and whatnot, when I saw your PR I did suspect that there would be more cases in which this breaks, but it didn't feel like a too big of a deal to me as I think that as long as we improve things/go in the right direction it makes sense to fix the small bits we find without getting bugged into solving everything...
(but yeah it's probably better to at least start with a nice thought out robust solution instead of throwing things at the wall and see what sticks)

I'm happy to take another look later this week, but if you have an idea for how you want to best support this, please feel free.

Please do continue! I personally don't have an idea on how to tackle this on the top of my head 🙂
but anyways if you won't find the time or drop this I am always happy to step in and help 🙂

The current implementation also seems to assume that variables are always strings, but when you do foo=1 in vars, it's available under env as a number, and not a string.

I think you're right! good catch!

workers-sdk/packages/wrangler/src/type-generation.ts

Lines 152 to 158 in cab7e1c

	if (
	typeof varValue === "string" ||
	typeof varValue === "number" ||
	typeof varValue === "boolean"
	) {
	envTypeStructure.push(`${varName}: "${varValue}";`);
	}

but that's a separate bug, if you want please open a new issue for that and we can tackle that separately 🙂 (I don't mind doing it if you want)
(it should also be pretty straightforward to handle)

Yeah that's definitely wrong!

You can do fun stuff like "breaking /*" = "testing" with the current implementation and break the rest of the generation too (very edge-casey).

😨 😬

[DaniFoldi]

I agree with @dario-piotrowicz that preferably wrangler shouldn't start quoting all props, since that would suddenly change all previously generated d.ts files just by updating wrangler and running the command.

If we go further in preferences, should it use single ' or double " quotes for keys? What about string literals?

[dario-piotrowicz]

If we go further in preferences, should it use single ' or double " quotes for keys? What about string literals?

I also do prefer single quotes in js/ts code 😄, but in this case, changing that would again suddenly change all previously generated d.ts files, wouldn't it? so I guess that given that double quotes have been used thus far I'd just stick with those 😕

[Cherry]

With the latest commits, this should be good for review. It'll (naively) check if an identifier is valid in JS or not, and only if it's not, will it then quote it.

It doesn't cover every single edge-case, especially when it comes to weird UTF8 characters, but it handles what I'd consider the vast vast majority in the real-world. I've added tests for the functions I've added, and abstracted them so they can be improved in future as needed.

[dario-piotrowicz]

Looks good to me 🙂

Except a few nits that I hope get addressed

Also I personally dislike tsdoc comments in imperative form (e.g. check ...) and prefer them in third person (e.g. checks ...), I'd push back on them but I've checked and in the whole codebase it seems we have a mix of both (and imperative even looks like the more common 🥲), so I'm fine with them.

Note

I've tested the prerelease and it does look like it improves the situation but as @Cherry said, it doesn't cover everything

[DaniFoldi]

I found an edge case which breaks the literal type escaping introduced in this PR:

[vars]
VARIABLE = 'some-"string\\'

[Cherry]

I found an edge case which breaks the literal type escaping introduced in this PR:

[vars]
VARIABLE = 'some-"string\\'

Yeah I would definitely consider that one of the edge-cases here that's not worth fixing.

---

My main goal in this PR was to handle the common things like dashes, numbers at start, etc. Given how flexible toml is, there's likely going to be lots of ways to break this, but with the changes here, it's a lot less fragile than the current release.

[codecov]

Codecov Report

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

Project coverage is 72.22%. Comparing base (7d160c7) to head (98d6048).
Report is 67 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5091      +/-   ##
==========================================
- Coverage   72.29%   72.22%   -0.07%     
==========================================
  Files         318      332      +14     
  Lines       16458    17266     +808     
  Branches     4201     4413     +212     
==========================================
+ Hits        11898    12471     +573     
- Misses       4560     4795     +235     

Files	Coverage Δ
packages/wrangler/src/type-generation.ts	98.75% <97.56%> (-0.54%)	⬇️

... and 99 files with indirect coverage changes

[Cherry]

Tests all passing now, woot! 🥳 Should be ready for a final review and then merge @dario-piotrowicz, thanks!

[dario-piotrowicz]

@Cherry looks good to me! thanks a bunch for the improvement! 😄

Unfortunately an approval from wrangler is still needed 😓, I've posted this PR in the internal channel to get someone to review it 🙂👍

[Cherry]

With cf-typegen being a growing thing in many templates, it would be good to see this merged in soon if possible. 👀