Untangle `wrangler` types 🙈 #2377

mrbbot · 2022-12-09T13:13:25Z

Whilst fixing another bug, I noticed fetch was in the global typing environment for wrangler. This shouldn't be the case, as our minimum supported Node version is 16.13.0, which doesn't include global fetch. Digging deeper, I noticed lots of files were being typed with a mix of @cloudflare/workers-types, node and jest types. This meant we could happily write a Jest describe in Wrangler CLI code, and TypeScript wouldn't complain. Not good... 😅

This PR splits wrangler's directories into separate TypeScript projects, each with their own configuration, and makes sure we aren't importing files that pollute global types. This ensures files get type checked under the correct environment.

A new tsc-all.js script has been added that automatically runs tsc on each project. tsconfig-santiy.ts files have also been added with @ts-expect-error comments to make sure this doesn't happen again.

I've also fixed the type checking of fixtures, and incorporated this into our root check script.

changeset-bot · 2022-12-09T13:13:28Z

🦋 Changeset detected

Latest commit: 56a15ed

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

This PR includes changesets to release 1 package

Name	Type
wrangler	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

packages/wrangler/scripts/bundle.ts

packages/wrangler/src/__tests__/tsconfig-sanity.ts

packages/wrangler/src/dev/dev.tsx

petebacondarwin · 2022-12-09T14:30:43Z

packages/wrangler/src/pages/dev.tsx

@@ -541,7 +541,7 @@ export const Handler = async ({

 	CLEANUP_CALLBACKS.push(stop);

-	waitUntilExit().then(() => {
+	void waitUntilExit().then(() => {


Perhaps this should just use a .finally instead?

Probably, but we aren't really doing anything with the Promise

Hmmm, if this throws, we probably don't want to exit with code 0? Unhandled rejections will trigger the process.on("exit") listener below too.

fixtures/local-mode-tests/tests/nodeBuiltinPackage.test.ts

github-actions · 2022-12-09T14:43:27Z

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/runs/3685450343/npm-package-wrangler-2377

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

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

Or you can use npx with this latest build directly:

npx https://prerelease-registry.devprod.cloudflare.dev/runs/3685450343/npm-package-wrangler-2377 dev path/to/script.js

Additional artifacts:

npm install https://prerelease-registry.devprod.cloudflare.dev/runs/3685450343/npm-package-cloudflare-pages-shared-2377

codecov · 2022-12-09T14:49:19Z

Codecov Report

Merging #2377 (56a15ed) into main (4bdb1f6) will increase coverage by 0.75%.
The diff coverage is 33.33%.

@@            Coverage Diff             @@
##             main    #2377      +/-   ##
==========================================
+ Coverage   70.71%   71.46%   +0.75%     
==========================================
  Files         153      153              
  Lines        9751     9644     -107     
  Branches     2550     2515      -35     
==========================================
- Hits         6895     6892       -3     
+ Misses       2856     2752     -104

Impacted Files	Coverage Δ
packages/wrangler/src/api/dev.ts	`69.23% <ø> (ø)`
packages/wrangler/src/dev.tsx	`84.91% <ø> (ø)`
packages/wrangler/src/dev/start-server.ts	`62.58% <0.00%> (ø)`
packages/wrangler/src/init.ts	`94.56% <ø> (-0.91%)`	⬇️
packages/wrangler/src/pages/dev.tsx	`19.02% <0.00%> (ø)`
packages/wrangler/src/dev/local.tsx	`22.14% <20.00%> (-0.17%)`	⬇️
packages/wrangler/src/dispatch-namespace.ts	`100.00% <100.00%> (ø)`
packages/wrangler/src/pages/upload.tsx	`83.43% <100.00%> (ø)`
.../wrangler/src/__tests__/helpers/msw/handlers/r2.ts	`52.94% <0.00%> (-11.77%)`	⬇️
... and 8 more

penalosa · 2022-12-09T17:51:41Z

I was joking when I said to try and change more files... 😂

.eslintrc.js

fixtures/local-mode-tests/src/module.ts

fixtures/node-app-pages/functions/stripe.ts

fixtures/remix-pages-app/.gitignore

packages/wrangler/templates/middleware/loader-sw.ts

packages/wrangler/templates/pages-dev-pipeline.ts

penalosa · 2022-12-09T18:30:49Z

packages/wrangler/templates/pages-shim.ts

-		const response = await env.ASSETS.fetch(request.url, request);
+		const response = await env.ASSETS.fetch(
+			request.url,
+			request as RequestInit<RequestInitCfProperties>


Why is this cast needed?

This is probably a bug in @cloudflare/workers-types tbh. The cf types from the fetch handler request parameter and global fetch were incompatible.

packages/wrangler/templates/pages-template-plugin.ts

packages/wrangler/templates/pages-template-worker.ts

penalosa

Once the cf casting issue is fixed, this looks good to go!

fixtures/local-mode-tests/tests/ports.test.ts

JacobMGEvans · 2022-12-12T14:56:47Z

packages/wrangler/src/__tests__/r2.test.ts

@@ -332,7 +332,7 @@ describe("r2", () => {
 					  "host": "api.cloudflare.com",
 					}
 				`);
-						response.once(
+						return response.once(


ugh... ty for catching these, little disappointed the tests were passing without them returning the response properly, and I overlooked them.

Hmmm, would you be able to have a look and see what's going on here?

After I finishing converting the other tests, for sure.

Likely something like this in these tests too #2377 (comment)

JacobMGEvans · 2022-12-12T15:01:43Z

packages/wrangler/templates/pages-template-plugin.ts

@@ -119,7 +119,7 @@ function* executeRequest(request: Request, relativePathname: string) {
 	}
 }

-export default function (pluginArgs) {
+export default function (pluginArgs: unknown) {


I am guessing this was undefined before

It was implicitly any typed, which was causing a TypeScript error:

mrbbot · 2022-12-13T13:13:06Z

Once the cf casting issue is fixed, this looks good to go!

@penalosa In the interest of avoiding this PR going stale, and bearing in mind the time it will take to get a workers-types release out with this fix, would you mind if we merge this now? I've replaced the cast with a @ts-expect-error, so we'll get a type error when we fix this.

penalosa · 2022-12-13T15:03:42Z

Yeah, I'm good with that—can you make an issue to fix? (I know the ts-expect-error will catch it, but just for tracking)

mrbbot · 2022-12-13T15:08:17Z

Updated #2390 👍

mrbbot requested review from a team as code owners December 9, 2022 13:13

petebacondarwin reviewed Dec 9, 2022

View reviewed changes

packages/wrangler/scripts/bundle.ts Outdated Show resolved Hide resolved

mrbbot marked this pull request as draft December 9, 2022 14:25

petebacondarwin reviewed Dec 9, 2022

View reviewed changes

packages/wrangler/src/__tests__/tsconfig-sanity.ts Show resolved Hide resolved

petebacondarwin reviewed Dec 9, 2022

View reviewed changes

packages/wrangler/src/dev/dev.tsx Show resolved Hide resolved

petebacondarwin reviewed Dec 9, 2022

View reviewed changes

fixtures/local-mode-tests/tests/nodeBuiltinPackage.test.ts Show resolved Hide resolved

mrbbot force-pushed the bcoll/untangle-types branch 3 times, most recently from 46cfa63 to 7942c1d Compare December 9, 2022 17:21

mrbbot marked this pull request as ready for review December 9, 2022 17:49

penalosa reviewed Dec 9, 2022

View reviewed changes

mrbbot mentioned this pull request Dec 12, 2022

Tidy up types in templates #2390

Closed

mrbbot force-pushed the bcoll/untangle-types branch from 7942c1d to 7a99683 Compare December 12, 2022 11:19

mrbbot requested a review from penalosa December 12, 2022 11:21

penalosa approved these changes Dec 12, 2022

View reviewed changes

JacobMGEvans reviewed Dec 12, 2022

View reviewed changes

fixtures/local-mode-tests/tests/ports.test.ts Show resolved Hide resolved

JacobMGEvans reviewed Dec 12, 2022

View reviewed changes

JacobMGEvans approved these changes Dec 12, 2022

View reviewed changes

mrbbot added 3 commits December 13, 2022 12:36

Untangle wrangler types

2402561

Fix fixture types

74c7a49

Upgrade remix-pages-app fixture's Remix version

56a15ed

mrbbot force-pushed the bcoll/untangle-types branch from 7a99683 to 56a15ed Compare December 13, 2022 12:36

mrbbot merged commit 32686e4 into main Dec 13, 2022

mrbbot deleted the bcoll/untangle-types branch December 13, 2022 15:08

github-actions bot mentioned this pull request Dec 13, 2022

Version Packages #2394

Merged

mrbbot mentioned this pull request Dec 19, 2022

Implement wrangler dev --(experimental-)local E2E tests #2365

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Untangle `wrangler` types 🙈 #2377

Untangle `wrangler` types 🙈 #2377

mrbbot commented Dec 9, 2022

changeset-bot bot commented Dec 9, 2022 •

edited

Loading

petebacondarwin Dec 9, 2022

JacobMGEvans Dec 9, 2022

mrbbot Dec 9, 2022

github-actions bot commented Dec 9, 2022 •

edited

Loading

codecov bot commented Dec 9, 2022 •

edited

Loading

penalosa commented Dec 9, 2022

penalosa Dec 9, 2022

mrbbot Dec 9, 2022

penalosa left a comment

JacobMGEvans Dec 12, 2022 •

edited

Loading

mrbbot Dec 12, 2022

JacobMGEvans Dec 12, 2022

JacobMGEvans Dec 12, 2022

JacobMGEvans Dec 12, 2022

mrbbot Dec 12, 2022

JacobMGEvans Dec 12, 2022

mrbbot commented Dec 13, 2022

penalosa commented Dec 13, 2022

mrbbot commented Dec 13, 2022

Untangle wrangler types 🙈 #2377

Untangle wrangler types 🙈 #2377

Conversation

mrbbot commented Dec 9, 2022

changeset-bot bot commented Dec 9, 2022 • edited Loading

🦋 Changeset detected

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Dec 9, 2022 • edited Loading

codecov bot commented Dec 9, 2022 • edited Loading

Codecov Report

penalosa commented Dec 9, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

penalosa left a comment

Choose a reason for hiding this comment

JacobMGEvans Dec 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrbbot commented Dec 13, 2022

penalosa commented Dec 13, 2022

mrbbot commented Dec 13, 2022

Untangle `wrangler` types 🙈 #2377

Untangle `wrangler` types 🙈 #2377

changeset-bot bot commented Dec 9, 2022 •

edited

Loading

github-actions bot commented Dec 9, 2022 •

edited

Loading

codecov bot commented Dec 9, 2022 •

edited

Loading

JacobMGEvans Dec 12, 2022 •

edited

Loading