Add strict null checks to the codebase by petebacondarwin · Pull Request #233 · cloudflare/workers-sdk

petebacondarwin · 2022-01-11T16:57:18Z

Also converts incorrect use of void to undefined and adds some words to the CSpell dictionary.

changeset-bot · 2022-01-11T16:57:51Z

⚠️ No Changeset found

Latest commit: 4b62761

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

petebacondarwin · 2022-01-11T17:04:03Z

It is a shame that TS cannot infer this.

could include in the while check a && Array.isArray(searchPaths) it could be not inferring because .length can also be on a string.

or alternatively

Suggested change

const cwd = searchPaths.shift()!;

const cwd = Array.isArray(searchPaths) && searchPaths.shift()!;

If we run across a lot of these we can make a custom type-guard, this example isn't inferring anything from inputs or using generics.

function isNonEmptyArrStrings(value: any): boolean { return Array.isArray(value) && value.length && value.every(item => typeof item === "string"); }

I'm going to add the guard because that is quite cool

petebacondarwin · 2022-01-11T17:04:50Z

There is no need to check the value (according to the types) since it will always be defined.

Always present, but not always truthy. Once the iterator is exhausted, value will be undefined.

Doh! I should have remembered.

So the "proper" way should be:

const {value, done} = handleIterator.next(); if (!done) { ... }

But then that would mean that we don't handle the final return statement from the executeRequest() generator.

So I can see that this is the most appropriate approach. Thanks for catching this.
(If only we had unit tests for this...)

Or we could do:

const context: EventContext<unknown, string, Record<string, unknown>> = { request: new Request(request.clone()), next: done ? () => {} : next, params, data, env, waitUntil: workerContext.waitUntil.bind(workerContext), };

But that is a bit less easy to grok, perhaps?

Oh actually, I remember why we needed to remove this.
If we have the if (value) then we must also have an else clause that returns "something". Otherwise the return type of next() becomes Promise<Response|undefined> which is not compatible with the EventContext type.

@GregBrimble - can you take a new look at this file now? I have refactored the code a bit.

petebacondarwin · 2022-01-11T17:05:16Z

Should this be a named type?

I think it would clarify the API if it was

petebacondarwin · 2022-01-11T17:06:47Z

It is possible that the previousBundle is undefined. In this case I have chosen to just return it unchanged. Is that a good idea? cc @threepointone

I would rather assert that it is defined, and throw if not. (In practice it will never be undefined, because this will be called only after the first build) Alternately, a non-null assertion?

I'll add an error.
But I don't actually understand what the point of this handler is. Is the idea that updating bundle by calling setBundle() causes the Dev component to be re-rendered?

petebacondarwin · 2022-01-11T17:09:46Z

this is set to wsServerRef.current but not if we use a fat arrow...

that's strange.

threepointone

Left a first round of comments

threepointone · 2022-01-11T18:41:00Z

I would rather assert that it is defined, and throw if not. (In practice it will never be undefined, because this will be called only after the first build) Alternately, a non-null assertion?

threepointone · 2022-01-11T18:43:44Z

I don't think we want to return here, instead we'll have to create the object and copy the inherited fields on to it.

The previous code would crash if the value of config.env[key] was undefined, since it had the following unguarded expression: config.env[env] (where in that case env is the key.

threepointone · 2022-01-11T18:44:50Z

same here, I think we don't want to return early

Similar to #233 (comment) - the previous code would crash in this case.

should it be an error in both cases then?
throw new Error('Missing ${something} in environments)

threepointone · 2022-01-11T18:47:41Z

why did you remove the args.local checks here?

Two reasons:

they are redundant due to the error being thrown earlier in the function if args.local is true

if this block is wrapped in an if statement then TS is unable to tell that config.account_id is not undefined in the fetchResult() call below.

threepointone · 2022-01-11T18:49:13Z

using <filename> (i.e the pointy brackets) implies demandOption:true, but ok.

That is true for "runtime" but for type checking the typings are only able to indicate that args.filename must not be undefined if the demandOption property is true.

Didn't we recently add the PR from James that allowed for empty filename to attempt to run on a root index? This would override that.

#79

The PR you link to was replaced by #196.
Even in this new PR the filename option is still required, because that is passed to esbuild as its entry point.

Hmmm, James had mentioned it defaulting to index the 196 PR will be merged soon and we can check the behavior with this change too

threepointone · 2022-01-11T18:51:13Z

I think we should assert here. It's a big problem if it's not defined, so we should probably throw.

An alternative is that we capture these values in a const outside the callback scope...

const serverInst = serverRef.current; const wsServerInst = wsServerRef.current; return () => { serverInst.close(); // Also disconnect any open websockets/devtools connections wsServerInst.clients.forEach((ws) => ws.close()); wsServerInst.close(); };

But I wasn't sure if we could be confident that this would never change between calls to useEffect() and its cleanup function...

JacobMGEvans · 2022-01-12T20:22:21Z

seems like we can remove import assert from "assert"; with current changes

GregBrimble · 2022-01-13T13:11:59Z

Suggested change

"Trying to fetch assets directly when there is no `directory` option specified, and not in `local` mode."

"Trying to fetch assets directly when there is no `directory` option specified."

GregBrimble · 2022-01-13T13:13:57Z

Tried to add a couple of commits, but realized this is on your fork, @petebacondarwin .

So, two tiny change requests:

--- a/packages/wrangler/src/pages.tsx
+++ b/packages/wrangler/src/pages.tsx
@@ -1,6 +1,5 @@
 /* eslint-disable no-shadow */
 
-import assert from "assert";
 import type { BuilderCallback } from "yargs";
 import { join } from "path";
 import { tmpdir } from "os";
@@ -122,7 +121,9 @@ async function spawnProxyProcess({
       },
     }
   );
-  EXIT_CALLBACKS.push(() => proxy.kill());
+  EXIT_CALLBACKS.push(() => {
+    proxy.kill();
+  });

Other than that, the Pages pieces look good to me! Happy to merge.

petebacondarwin · 2022-01-13T15:00:22Z

@GregBrimble as a "collaborator" on this repo, you have the rights to push to this branch on my fork.

`void` should only be used for return types of functions that do not return anything (not even undefined). See https://www.typescriptlang.org/docs/handbook/2/functions.html#void

The previous fix (removing the `if(value)` check actually broke the behaviour of the code.

petebacondarwin · 2022-01-15T17:07:21Z

Closing and replacing by:

petebacondarwin requested a review from threepointone as a code owner January 11, 2022 16:57

petebacondarwin commented Jan 11, 2022

View reviewed changes

Comment thread packages/wrangler/pages/functions/filepath-routing.ts Outdated

petebacondarwin commented Jan 11, 2022

View reviewed changes

petebacondarwin force-pushed the strict-null-checks branch from e32566a to 42da8a2 Compare January 11, 2022 17:09

petebacondarwin commented Jan 11, 2022

View reviewed changes

threepointone reviewed Jan 11, 2022

View reviewed changes

petebacondarwin force-pushed the strict-null-checks branch from 42da8a2 to c7aa4ae Compare January 12, 2022 13:50

JacobMGEvans reviewed Jan 12, 2022

View reviewed changes

Comment thread packages/wrangler/pages/functions/routes.ts Outdated

JacobMGEvans reviewed Jan 12, 2022

View reviewed changes

GregBrimble reviewed Jan 13, 2022

View reviewed changes

Comment thread packages/wrangler/pages/functions/filepath-routing.ts Outdated

GregBrimble reviewed Jan 13, 2022

View reviewed changes

petebacondarwin added 4 commits January 13, 2022 16:28

Use undefined rather than void

eead94c

`void` should only be used for return types of functions that do not return anything (not even undefined). See https://www.typescriptlang.org/docs/handbook/2/functions.html#void

Ignore common technical words

0b0ef4e

Enable TypeScript strictNullChecks across the codebase

43b330f

refactor template-worker to handle strict null checks correctly

4b62761

The previous fix (removing the `if(value)` check actually broke the behaviour of the code.

petebacondarwin force-pushed the strict-null-checks branch from c7aa4ae to 4b62761 Compare January 13, 2022 16:50

petebacondarwin marked this pull request as draft January 14, 2022 12:28

petebacondarwin added the cleanup label Jan 14, 2022

petebacondarwin closed this Jan 15, 2022

petebacondarwin deleted the strict-null-checks branch January 19, 2022 13:47

workers-devprod added this to workers-sdk Sep 28, 2025

github-project-automation bot moved this to Untriaged in workers-sdk Sep 28, 2025

	const cwd = searchPaths.shift()!;
	const cwd = Array.isArray(searchPaths) && searchPaths.shift()!;

	"Trying to fetch assets directly when there is no `directory` option specified, and not in `local` mode."
	"Trying to fetch assets directly when there is no `directory` option specified."

Conversation

petebacondarwin commented Jan 11, 2022

Uh oh!

changeset-bot bot commented Jan 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

⚠️ No Changeset found

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

threepointone left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

petebacondarwin Jan 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

changeset-bot bot commented Jan 11, 2022 •

edited

Loading

petebacondarwin Jan 13, 2022 •

edited

Loading

GregBrimble commented Jan 13, 2022 •

edited

Loading