chore(node): stricter types #900

kanadgupta · 2023-08-14T21:50:29Z

🧰 Changes

As part of QA'ing Marc's changes in #879, I noticed our TypeScript config for the Node SDK is pretty loosey-goosey so just tightening it up a bit!

🧬 QA & Testing

~~No actual code changes~~ EDIT: as of 802a65c, there are minor code changes (still seemingly harmless, mostly fallback values in a few places). Do the TS build + tests still pass?

i had to downgrade to npm@7 for this... absolutely cursed

packages/node/test/lib/is-request.test.ts

packages/node/tsconfig.json

packages/node/test/tsconfig.json

packages/node/src/lib/metrics-log.ts

packages/node/test/lib/verify-webhook.test.ts

not sure why VS code didn't flag these the first time 🤔 also i know this is nuts but open to alternatives lol

Co-Authored-By: Jon Ursenbach <[email protected]>

this is the shit i live for

packages/node/src/lib/object-to-array.ts

packages/node/src/lib/patch-response.ts

packages/node/src/lib/process-request.ts

llimllib · 2023-08-15T13:31:15Z

packages/node/src/lib/process-request.ts

  }

-  return requestData;
+  return requestData as Request;


You could remove this assertion if we can pass castToString for headers and make method a string:

const requestData = { - method: req.method, + method: req.method || '', url: url.format({ // Handle cases where some reverse proxies put two protocols into x-forwarded-proto // This line does the following: "https,http" -> "https" @@ -206,7 +206,7 @@ export default function processRequest( query: qs.parse(reqUrl.search.substring(1)), }), httpVersion: `${getProto(req).toUpperCase()}/${req.httpVersion}`, - headers: objectToArray(req.headers), + headers: objectToArray(req.headers, { castToString: true }), queryString: searchToArray(reqUrl.searchParams), postData, // TODO: When readme starts accepting these, send the correct values @@ -221,5 +221,5 @@ export default function processRequest( return remainingRequestData as Request; } - return requestData as Request; + return requestData; }

I'm not 100% sure it's valid to convert the headers into strings? ISTM that it probably is but I don't know - I just dislike using type assertions when possible, and I'd rather change our return type if headers may not be a string than to just promise that it is and then have it not be at runtime

yeah I like this suggestion but I'm wondering if there was a deliberate reason why we're not setting that castToString flag here. again this is another example of being unsure of what our ingestion service likes

I'd be pretty concerned if we passed { method: '' }

Riiiight, but for some reason req.method can be undefined per the official Node types 🤔 I'll leave that fallback in there for now knowing that it probably won't get hit at all, but let me know if you disagree here!

ok on second thought I decided to enable the castToString flag on this line:

metrics-sdks/packages/node/src/lib/process-request.ts

Line 209 in 1585f05

headers: objectToArray(req.headers, { castToString: true }),

is this a safe change to make @gratcliff? see the full commit here: 1585f05

packages/node/src/lib/process-response.ts

packages/node/src/lib/metrics-log.ts

packages/node/src/lib/process-request.ts

Co-Authored-By: Bill Mill <[email protected]>

packages/node/src/lib/process-request.ts

Co-Authored-By: Bill Mill <[email protected]>

gratcliff · 2023-08-15T15:02:20Z

packages/node/src/lib/metrics-log.ts

@@ -34,21 +34,23 @@ export interface GroupingObject {
 export interface OutgoingLogBody {
  _id?: string;
  _version: number;
-  clientIPAddress: string;
+  clientIPAddress?: string;


Are we expecting this to be null occasionally? The value should always be here, right?

updated this type in af9ce2a, what do you think? this is happening because req.socket.remoteAddress can be undefined on this line:

metrics-sdks/packages/node/src/lib/construct-payload.ts

Line 151 in af9ce2a

clientIPAddress: req.socket.remoteAddress || '',

packages/node/src/lib/metrics-log.ts

gratcliff · 2023-08-15T15:04:44Z

packages/node/src/lib/process-request.ts

  }

-  return requestData;
+  return requestData as Request;


I'd be pretty concerned if we passed { method: '' }

packages/node/src/lib/process-request.ts

Dashron · 2023-08-15T15:49:48Z

packages/node/src/lib/process-request.ts

@@ -184,7 +187,7 @@ export default function processRequest(
  // We use a fake host here because we rely on the host header which could be redacted.
  // We only ever use this reqUrl with the fake hostname for the pathname and querystring.
  // req.originalUrl is express specific, req.url is node.js
-  const reqUrl = new URL(req.originalUrl || req.url, 'https://readme.io');
+  const reqUrl = new URL(req.originalUrl || req.url || '', 'https://readme.io');


is empty string really valid here, or is it just to cover the possibility of a falsey type? If it's not a real possibility here it might be better to throw an error early so that the user is notified of an issue with urls.

yeah I think there are opportunities to rework the logic to throw better validation errors (yay TS strict mode!) but I'm trying to confine the scope of the changes in this PR to fallback values where needed and avoid any bigger logic changes

packages/node/src/lib/process-request.ts

Co-Authored-By: Aaron Hedges <[email protected]>

gratcliff · 2023-08-16T16:11:29Z

packages/node/src/lib/metrics-log.ts

@@ -41,14 +41,16 @@ export interface OutgoingLogBody {
  request: Har;
 }

+type LogId = string | string[] | undefined;


If you wanted, you could include the uuid type here and consume it instead of string.

it was slightly annoying since the uuid npm package doesn't play nicely with Node's baked in UUID type, but i like this idea — see here! b5c3455

why are we using an npm package instead of randomUUID from node:crypto?

looks like it only got added in Node v19: https://developer.mozilla.org/en-US/docs/Web/API/Crypto/randomUUID#browser_compatibility

doh! thanks for nothing, ide auto-fill

(was the UUID type added before the uuid function?)

wait weird, apparently the Node.js docs say that randomUUID has been available since Node 14/15? https://nodejs.org/api/crypto.html#cryptorandomuuidoptions

so maybe we could refactor things to use that, at least once we drop support for Node 14

gratcliff · 2023-08-16T16:12:26Z

packages/node/src/lib/object-to-array.ts

+  value: unknown;
+}[];
+
+export function objectToArray(


Why export duplicate functions here?

it's called a function overload! since this function accepts an optional opts object, the first overload allows us to specify a more specific return type :chefs-kiss:

(this is literally my first time using them and I love 'em)

Co-Authored-By: Gabe Ratcliff <[email protected]>

kanadgupta added 2 commits August 14, 2023 15:59

chore(node): tidy up types

e1ad633

test: add better types to testbed

0acbc1c

kanadgupta added area:node refactor Issues about tackling technical debt labels Aug 14, 2023

chore: refresh lockfile

0ee8211

i had to downgrade to npm@7 for this... absolutely cursed

kanadgupta commented Aug 14, 2023

View reviewed changes

packages/node/test/lib/is-request.test.ts Show resolved Hide resolved

kanadgupta marked this pull request as ready for review August 14, 2023 22:06

kanadgupta requested review from erunion, domharrington and a team August 14, 2023 22:06

erunion reviewed Aug 14, 2023

View reviewed changes

packages/node/tsconfig.json Outdated Show resolved Hide resolved

chore: strict config fixes

802a65c

kanadgupta requested a review from erunion August 14, 2023 22:39

kanadgupta commented Aug 14, 2023

View reviewed changes

packages/node/test/tsconfig.json Show resolved Hide resolved

erunion reviewed Aug 14, 2023

View reviewed changes

packages/node/src/lib/metrics-log.ts Outdated Show resolved Hide resolved

packages/node/test/lib/verify-webhook.test.ts Show resolved Hide resolved

kanadgupta and others added 3 commits August 14, 2023 17:52

chore: more TS fixes

98c8232

not sure why VS code didn't flag these the first time 🤔 also i know this is nuts but open to alternatives lol

chore: use promise.resolve generics

a718dae

Co-Authored-By: Jon Ursenbach <[email protected]>

test: add test for undefined webhook

022afbc

Co-Authored-By: Jon Ursenbach <[email protected]>

kanadgupta requested a review from erunion August 14, 2023 22:58

erunion approved these changes Aug 14, 2023

View reviewed changes

kanadgupta added 2 commits August 14, 2023 18:02

refactor: consolidate type

097a84c

this is the shit i live for

chore: slightly reorganize tsconfigs

bc40804

kanadgupta requested a review from Dashron August 15, 2023 13:03

revert: var change

d931139

llimllib reviewed Aug 15, 2023

View reviewed changes

refactor: add'l stronger types

8e1b743

llimllib reviewed Aug 15, 2023

View reviewed changes

packages/node/src/lib/metrics-log.ts Outdated Show resolved Hide resolved

llimllib reviewed Aug 15, 2023

View reviewed changes

packages/node/src/lib/process-request.ts Show resolved Hide resolved

refactor: another strict type

9ee2fcc

Co-Authored-By: Bill Mill <[email protected]>

llimllib reviewed Aug 15, 2023

View reviewed changes

packages/node/src/lib/process-request.ts Outdated Show resolved Hide resolved

kanadgupta and others added 3 commits August 15, 2023 08:47

refactor: consolidate type

8288b96

Co-Authored-By: Bill Mill <[email protected]>

refactor: more type passes

ef396a9

Co-Authored-By: Bill Mill <[email protected]>

chore: better comment

7d42eac

gratcliff approved these changes Aug 15, 2023

View reviewed changes

Dashron reviewed Aug 15, 2023

View reviewed changes

packages/node/src/lib/process-request.ts Outdated Show resolved Hide resolved

Dashron reviewed Aug 15, 2023

View reviewed changes

packages/node/src/lib/process-request.ts Outdated Show resolved Hide resolved

kanadgupta and others added 4 commits August 15, 2023 11:02

chore: another stricter type

03007f4

Co-Authored-By: Aaron Hedges <[email protected]>

refactor: clientIPAddress is required

af9ce2a

revert: oops didn't mean to commit this

d4e9622

refactor: stricter typing for headers and method

1585f05

kanadgupta requested a review from gratcliff August 15, 2023 21:11

kanadgupta added 2 commits August 16, 2023 08:45

chore: comment cleanup in tests

97bb826

chore: slightly stricter type

82f06dc

llimllib approved these changes Aug 16, 2023

View reviewed changes

gratcliff reviewed Aug 16, 2023

View reviewed changes

refactor: uuid type

b5c3455

Co-Authored-By: Gabe Ratcliff <[email protected]>

kanadgupta merged commit 9c35f38 into main Aug 16, 2023

kanadgupta deleted the fix/stricter-types branch August 16, 2023 17:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(node): stricter types #900

chore(node): stricter types #900

kanadgupta commented Aug 14, 2023 •

edited

Loading

llimllib Aug 15, 2023 •

edited

Loading

kanadgupta Aug 15, 2023

gratcliff Aug 15, 2023

kanadgupta Aug 15, 2023

kanadgupta Aug 15, 2023 •

edited

Loading

gratcliff Aug 15, 2023

kanadgupta Aug 15, 2023

gratcliff Aug 15, 2023

Dashron Aug 15, 2023

kanadgupta Aug 15, 2023

gratcliff Aug 16, 2023

kanadgupta Aug 16, 2023

llimllib Aug 16, 2023

kanadgupta Aug 16, 2023

llimllib Aug 16, 2023

llimllib Aug 16, 2023

kanadgupta Aug 16, 2023

kanadgupta Aug 16, 2023

gratcliff Aug 16, 2023

kanadgupta Aug 16, 2023

chore(node): stricter types #900

chore(node): stricter types #900

Conversation

kanadgupta commented Aug 14, 2023 • edited Loading

🧰 Changes

🧬 QA & Testing

llimllib Aug 15, 2023 • 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

kanadgupta Aug 15, 2023 • 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kanadgupta commented Aug 14, 2023 •

edited

Loading

llimllib Aug 15, 2023 •

edited

Loading

kanadgupta Aug 15, 2023 •

edited

Loading