-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Add strict null checks to the codebase #233
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.
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
Changes from all commits
eead94c
0b0ef4e
43b330f
4b62761
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -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) { | ||||||
| exportNames.push(declaration.id.name); | ||||||
| } | ||||||
|
|
||||||
|
|
@@ -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.
Outdated
|
||||||
| const method = parts.pop() ?? null; | ||||||
|
|
||||||
| const segments = segmentedPath.slice(1).split("/").filter(Boolean); | ||||||
| return [method, segments]; | ||||||
|
|
@@ -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()!; | ||||||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is a shame that TS cannot infer this.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. could include in the
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. or alternatively
Suggested change
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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");
}
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 | ||
|
|
@@ -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) { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Always present, but not always truthy. Once the iterator is exhausted,
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 So I can see that this is the most appropriate approach. Thanks for catching this.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh actually, I remember why we needed to remove this.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, | ||
|
|
@@ -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); | ||
| } | ||
| }; | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 }[] = []; | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this be a named type?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 }); | ||
| } | ||
|
|
@@ -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; | ||
| } | ||
| ); | ||
|
|
@@ -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( | ||
|
|
@@ -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); | ||
| } | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.