Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add strict null checks to the codebase #233

Closed
wants to merge 4 commits into from
Closed

Add strict null checks to the codebase #233

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
eead94c
Use `undefined` rather than `void`
petebacondarwin Jan 10, 2022
0b0ef4e
Ignore common technical words
petebacondarwin Jan 10, 2022
43b330f
Enable TypeScript strictNullChecks across the codebase
petebacondarwin Jan 11, 2022
4b62761
refactor template-worker to handle strict null checks correctly
petebacondarwin Jan 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 15 additions & 2 deletions .vscode/settings.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,27 @@
"editor.defaultFormatter": "esbenp.prettier-vscode",
"cSpell.words": [
"cfetch",
"clipboardy",
"cloudflared",
"esbuild",
"estree",
"execa",
"extensionless",
"iarna",
"keyvalue",
"middlewares",
"Miniflare",
"outdir",
"outfile",
"Positionals"
]
"pgrep",
"PKCE",
"Positionals",
"undici",
"wasmvalue",
"weakmap",
"weakset",
"webassemblymemory",
"websockets"
],
"cSpell.ignoreWords": ["yxxx"]
}
16 changes: 8 additions & 8 deletions packages/wrangler/pages/functions/filepath-routing.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ export async function generateConfigFromFileTree({
const declaration = node.declaration;

// `export async function onRequest() {...}`
if (declaration.type === "FunctionDeclaration") {
if (declaration.type === "FunctionDeclaration" && declaration.id) {
petebacondarwin marked this conversation as resolved.
Show resolved Hide resolved
exportNames.push(declaration.id.name);
}

Expand Down Expand Up @@ -155,12 +155,10 @@ export async function generateConfigFromFileTree({
// more specific routes aren't occluded from matching due to
// less specific routes appearing first in the route list.
export function compareRoutes(a: string, b: string) {
function parseRoutePath(routePath: string) {
let [method, segmentedPath] = routePath.split(" ");
if (!segmentedPath) {
segmentedPath = method;
method = null;
}
function parseRoutePath(routePath: string): [string | null, string[]] {
const parts = routePath.split(" ", 2);
const segmentedPath = parts.pop() ?? "";
petebacondarwin marked this conversation as resolved.
Show resolved Hide resolved
const method = parts.pop() ?? null;

const segments = segmentedPath.slice(1).split("/").filter(Boolean);
return [method, segments];
Expand Down Expand Up @@ -205,7 +203,9 @@ async function forEachFile<T>(
const returnValues: T[] = [];

while (searchPaths.length) {
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()!;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a shame that TS cannot infer this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or alternatively

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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");
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const dir = await fs.readdir(cwd, { withFileTypes: true });
for (const entry of dir) {
const pathname = path.join(cwd, entry.name);
Expand Down
2 changes: 1 addition & 1 deletion packages/wrangler/pages/functions/routes.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ export function parseConfig(config: Config, baseDir: string) {
});
}

for (const [route, props] of Object.entries(config.routes)) {
for (const [route, props] of Object.entries(config.routes ?? {})) {
petebacondarwin marked this conversation as resolved.
Show resolved Hide resolved
let [_methods, routePath] = route.split(" ");
if (!routePath) {
routePath = _methods;
Expand Down
39 changes: 15 additions & 24 deletions packages/wrangler/pages/functions/template-worker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,17 +33,16 @@ declare const routes: RouteHandler[];
declare const __FALLBACK_SERVICE__: string;

// expect an ASSETS fetcher binding pointing to the asset-server stage
type Env = {
[name: string]: unknown;
ASSETS: { fetch(url: string, init: RequestInit): Promise<Response> };
type FetchEnv = {
[name: string]: { fetch: typeof fetch };
ASSETS: { fetch: typeof fetch };
};

type WorkerContext = {
waitUntil: (promise: Promise<unknown>) => void;
};

// eslint-disable-next-line @typescript-eslint/no-unused-vars -- `env` can be used by __FALLBACK_SERVICE_FETCH__
function* executeRequest(request: Request, env: Env) {
function* executeRequest(request: Request, _env: FetchEnv) {
const requestPath = new URL(request.url).pathname;

// First, iterate through the routes (backwards) and execute "middlewares" on partial route matches
Expand Down Expand Up @@ -88,35 +87,21 @@ function* executeRequest(request: Request, env: Env) {
break;
}
}

// Finally, yield to the fallback service (`env.ASSETS.fetch` in Pages' case)
return {
handler: () =>
__FALLBACK_SERVICE__
? // @ts-expect-error expecting __FALLBACK_SERVICE__ to be the name of a service binding, so fetch should be defined
env[__FALLBACK_SERVICE__].fetch(request)
: fetch(request),
params: {} as Params,
};
}

export default {
async fetch(request: Request, env: Env, workerContext: WorkerContext) {
async fetch(request: Request, env: FetchEnv, workerContext: WorkerContext) {
const handlerIterator = executeRequest(request, env);
const data = {}; // arbitrary data the user can set between functions
const next = async (input?: RequestInfo, init?: RequestInit) => {
if (input !== undefined) {
request = new Request(input, init);
}

const { value } = handlerIterator.next();
if (value) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const { handler, params } = value;
const context: EventContext<
unknown,
string,
Record<string, unknown>
> = {
const result = handlerIterator.next();
if (!result.done) {
const { handler, params } = result.value;
const context = {
request: new Request(request.clone()),
next,
params,
Expand All @@ -132,6 +117,12 @@ export default {
[101, 204, 205, 304].includes(response.status) ? null : response.body,
response
);
} else if (__FALLBACK_SERVICE__) {
// There are no more handlers so finish with the fallback service (`env.ASSETS.fetch` in Pages' case)
return env[__FALLBACK_SERVICE__].fetch(request);
} else {
// There was not fallback service so actually make the request to the origin.
return fetch(request);
}
};

Expand Down
14 changes: 9 additions & 5 deletions packages/wrangler/src/__tests__/kv.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 }[] = [];
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be a named type?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would clarify the API if it was

for (let i = 0; i < 550; i++) {
KVNamespaces.push({ title: "title-" + i, id: "id-" + i });
}
Expand Down Expand Up @@ -335,8 +335,12 @@ describe("wrangler", () => {
expect(namespaceId).toEqual(expectedNamespaceId);
expect(key).toEqual(expectedKey);
expect(body).toEqual(expectedValue);
expect(query.get("expiration")).toEqual(`${expiration}`);
expect(query.get("expiration_ttl")).toEqual(`${expirationTtl}`);
if (expiration) {
expect(query.get("expiration")).toEqual(`${expiration}`);
}
if (expirationTtl) {
expect(query.get("expiration_ttl")).toEqual(`${expirationTtl}`);
}
return null;
}
);
Expand Down Expand Up @@ -681,7 +685,7 @@ describe("wrangler", () => {
if (expectedKeys.length <= keysPerRequest) {
return createFetchResult(expectedKeys);
} else {
const start = parseInt(query.get("cursor")) || 0;
const start = parseInt(query.get("cursor") ?? "0") || 0;
const end = start + keysPerRequest;
const cursor = end < expectedKeys.length ? end : undefined;
return createFetchResult(
Expand Down Expand Up @@ -778,7 +782,7 @@ describe("wrangler", () => {

it("should make multiple requests for paginated results", async () => {
// Create a lot of mock keys, so that the fetch requests will be paginated
const keys = [];
const keys: string[] = [];
for (let i = 0; i < 550; i++) {
keys.push("key-" + i);
}
Expand Down
8 changes: 4 additions & 4 deletions packages/wrangler/src/__tests__/mock-cfetch.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import type { RequestInit } from "node-fetch";
import type { URLSearchParams } from "node:url";
import { URLSearchParams } from "node:url";
import { pathToRegexp } from "path-to-regexp";
import { CF_API_BASE_URL } from "../cfetch";
import type { FetchResult } from "../cfetch";
Expand All @@ -9,8 +9,8 @@ import type { FetchResult } from "../cfetch";
*/
export type MockHandler<ResponseType> = (
uri: RegExpExecArray,
init?: RequestInit,
queryParams?: URLSearchParams
init: RequestInit,
queryParams: URLSearchParams
) => ResponseType;

type RemoveMockFn = () => void;
Expand All @@ -32,7 +32,7 @@ const mocks: MockFetch<unknown>[] = [];
export async function mockFetchInternal(
resource: string,
init: RequestInit = {},
queryParams?: URLSearchParams
queryParams: URLSearchParams = new URLSearchParams()
) {
for (const { regexp, method, handler } of mocks) {
const resourcePath = new URL(resource, CF_API_BASE_URL).pathname;
Expand Down
2 changes: 1 addition & 1 deletion packages/wrangler/src/api/form_data.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ export function toMimeType(type: CfModuleType): string {
}
}

function toModule(module: CfModule, entryType?: CfModuleType): Blob {
function toModule(module: CfModule, entryType: CfModuleType): Blob {
const { type: moduleType, content } = module;
const type = toMimeType(moduleType ?? entryType);

Expand Down
12 changes: 6 additions & 6 deletions packages/wrangler/src/api/worker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,15 +112,15 @@ export interface CfWorkerInit {
/**
* The name of the worker.
*/
name: string | void;
name: string | undefined;
/**
* The entrypoint module.
*/
main: CfModule;
/**
* The list of additional modules.
*/
modules: void | CfModule[];
modules: undefined | CfModule[];
/**
* All the bindings
*/
Expand All @@ -130,10 +130,10 @@ export interface CfWorkerInit {
vars?: CfVars;
services?: CfService[];
};
migrations: void | CfDurableObjectMigrations;
compatibility_date: string | void;
compatibility_flags: void | string[];
usage_model: void | "bundled" | "unbound";
migrations: undefined | CfDurableObjectMigrations;
compatibility_date: string | undefined;
compatibility_flags: undefined | string[];
usage_model: undefined | "bundled" | "unbound";
}

/**
Expand Down
12 changes: 4 additions & 8 deletions packages/wrangler/src/cfetch/internal.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import fetch from "node-fetch";
import type { RequestInit, HeadersInit } from "node-fetch";
import fetch, { Headers } from "node-fetch";
import type { RequestInit } from "node-fetch";
import { getAPIToken, loginOrRefreshIfRequired } from "../user";

export const CF_API_BASE_URL =
Expand All @@ -21,7 +21,7 @@ export async function fetchInternal<ResponseType>(
): Promise<ResponseType> {
await requireLoggedIn();
const apiToken = requireApiToken();
const headers = cloneHeaders(init.headers);
const headers = new Headers(init.headers);
addAuthorizationHeader(headers, apiToken);

const queryString = queryParams ? `?${queryParams.toString()}` : "";
Expand Down Expand Up @@ -55,11 +55,7 @@ function requireApiToken(): string {
return apiToken;
}

function cloneHeaders(headers: HeadersInit): HeadersInit {
return { ...headers };
}

function addAuthorizationHeader(headers: HeadersInit, apiToken: string): void {
function addAuthorizationHeader(headers: Headers, apiToken: string): void {
if (headers["Authorization"]) {
throw new Error(
"The request already specifies an authorisation header - cannot add a new one."
Expand Down
2 changes: 1 addition & 1 deletion packages/wrangler/src/config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,5 +127,5 @@ export type Config = {
usage_model?: UsageModel; // inherited
// top level
build?: Build;
env?: { [envName: string]: void | Env };
env?: { [envName: string]: undefined | Env };
};
Loading