Add strict null checks to the codebase #233
Conversation
|
| const cwd = searchPaths.shift(); | ||
| // The `searchPaths.length` check above ensures that `searchPaths.shift()` is defined | ||
| // eslint-disable-next-line @typescript-eslint/no-non-null-assertion | ||
| const cwd = searchPaths.shift()!; |
There was a problem hiding this comment.
It is a shame that TS cannot infer this.
There was a problem hiding this comment.
could include in the while check a && Array.isArray(searchPaths) it could be not inferring because .length can also be on a string.
There was a problem hiding this comment.
or alternatively
| const cwd = searchPaths.shift()!; | |
| const cwd = Array.isArray(searchPaths) && searchPaths.shift()!; |
There was a problem hiding this comment.
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");
}There was a problem hiding this comment.
I'm going to add the guard because that is quite cool
| @@ -110,29 +110,23 @@ export default { | |||
| } | |||
|
|
|||
| const { value } = handlerIterator.next(); | |||
| if (value) { | |||
There was a problem hiding this comment.
There is no need to check the value (according to the types) since it will always be defined.
There was a problem hiding this comment.
Always present, but not always truthy. Once the iterator is exhausted, value will be undefined.
There was a problem hiding this comment.
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...)
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@GregBrimble - can you take a new look at this file now? I have refactored the code a bit.
| @@ -212,7 +212,7 @@ describe("wrangler", () => { | |||
|
|
|||
| it("should make multiple requests for paginated results", async () => { | |||
| // Create a lot of mock namespaces, so that the fetch requests will be paginated | |||
| const KVNamespaces = []; | |||
| const KVNamespaces: { title: string; id: string }[] = []; | |||
There was a problem hiding this comment.
Should this be a named type?
There was a problem hiding this comment.
I think it would clarify the API if it was
packages/wrangler/src/dev.tsx
Outdated
| })); | ||
| setBundle( | ||
| (previousBundle) => | ||
| previousBundle && { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
force-pushed
the
strict-null-checks
branch
from
e32566a
to
42da8a2
Compare
| wsServerRef.current.on("connection", (ws: WebSocket) => { | ||
| if (wsServerRef.current.clients.size > 1) { | ||
| wsServerRef.current.on("connection", function (ws: WebSocket) { | ||
| if (this.clients.size > 1) { |
There was a problem hiding this comment.
this is set to wsServerRef.current but not if we use a fat arrow...
packages/wrangler/src/dev.tsx
Outdated
| })); | ||
| setBundle( | ||
| (previousBundle) => | ||
| previousBundle && { |
There was a problem hiding this comment.
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?
| const environments = config.env ?? {}; | ||
| Object.values(environments).forEach((env) => { | ||
| if (env === undefined) { | ||
| return; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| Object.keys(config.env || {}).forEach((env) => { | ||
| Object.entries(environments).forEach(([envName, envValue]) => { | ||
| if (envValue === undefined) { | ||
| return; |
There was a problem hiding this comment.
same here, I think we don't want to return early
There was a problem hiding this comment.
Similar to #233 (comment) - the previous code would crash in this case.
There was a problem hiding this comment.
should it be an error in both cases then?
throw new Error('Missing ${something} in environments)
| console.log("Did not login, quitting..."); | ||
| return; | ||
| } | ||
| const loggedIn = await loginOrRefreshIfRequired(); |
There was a problem hiding this comment.
why did you remove the args.local checks here?
There was a problem hiding this comment.
Two reasons:
- they are redundant due to the error being thrown earlier in the function if
args.localis true - if this block is wrapped in an if statement then TS is unable to tell that
config.account_idis not undefined in thefetchResult()call below.
| .positional("filename", { | ||
| describe: "entry point", | ||
| type: "string", | ||
| demandOption: true, |
There was a problem hiding this comment.
using <filename> (i.e the pointy brackets) implies demandOption:true, but ok.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
| wsServerRef.current.clients.forEach((ws) => ws.close()); | ||
| wsServerRef.current.close(); | ||
| }; | ||
| if (serverRef.current) { |
There was a problem hiding this comment.
I think we should assert here. It's a big problem if it's not defined, so we should probably throw.
There was a problem hiding this comment.
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...
petebacondarwin
force-pushed
the
strict-null-checks
branch
from
42da8a2
to
c7aa4ae
Compare
| // We only define `proxyPort`, above, when there is no `directory` defined. | ||
| const assetsFetch = | ||
| directory !== undefined | ||
| ? await generateAssetsFetch(directory) |
There was a problem hiding this comment.
seems like we can remove import assert from "assert"; with current changes
|
|
||
| const invalidAssetsFetch: typeof fetch = () => { | ||
| throw new Error( | ||
| "Trying to fetch assets directly when there is no `directory` option specified, and not in `local` mode." |
There was a problem hiding this comment.
| "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." |
|
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. |
|
@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
force-pushed
the
strict-null-checks
branch
from
c7aa4ae
to
4b62761
Compare
Also converts incorrect use of
voidtoundefinedand adds some words to the CSpell dictionary.