feat(node): new unified code snippet for both the webhook and metrics

[mjcuva]

Redo the node code snippet for the webhook/metrics! We now have one code snippet that handles both cases super easily.

import { ReadMe } from 'readmeio'

const readme = ReadMe('API_KEY')

router.use(
  readme(async (req, getUser) => {
    // We pass in getUser which will call the correct function to fetch a 
    // user depending on what we have in the request
    const user = getUser({
      byEmail: getUserByEmail, // Function defined by them that returns a user given an email address
      byAPIKey: getUserByAPIKey, // Function defined by them that returns a user given an API key
      manualAPIKey: getAPIKey(req.headers.authorization), // Optional way to explicitly tell us the api key if we can't guess it
    });

    return {
      name: user.name,
      email: user.email,
      keys: user.apiKeys,
    };
  }, {
    development: process.env.NODE_ENV === 'development', // require development: true to show local testing page
  })
);

Other things:

• Added a /readme-setup page that walks users through making sure everything is configured properly. This page is only enabled if development: true to make sure that it won't be enabled in prod on accident.
• Handles all of the metrics config in additional options parameter (which is optional)
• Exported everything that was already there, so this is purely additive
• User provided function should return all of the information about the user, even if it's not relevant for the specific API call that is happening. This means we can have one set of config for the webhook and metrics and the sdk will just figure out what is relevant for the call to metrics.
• Makes a call to the ReadMe API to get the JWT Secret & stable version (for linking to the dash)

Update 1/31

• I renamed some of the exports to clean it up a bit. Now we just have a ReadMe export that you call to pass in the API Key. Before we had a separate auth export that was required to be called before the middelware, which was confusing. This also cleans up how you call it (no more readmeio.readme stuff). This matches Sentry's sdks closer which makes me feel better about it.

Testing

• Install readmeio@next to test the new snippet
• Docs on setup here: https://docs.readme.com/main/docs/unified-snippet-docs
• PR for readme's api here: https://github.com/readmeio/readme/pull/10819

Things to look out for:

• Do the docs make sense?
• Issues with copy in the local setup page?
• Testing various APIs (with various auth types)

[erunion]

does the sdk-snippets library need to get updated too?

[erunion]

@kanadgupta probably worth upgrading this to the latest msw

[ilias-t]

nit: is the not redundant when we already have src/**/*?

[mjcuva]

@kanadgupta added this so I'll let him chime in with more context!

[kanadgupta]

i think this is because tsconfig ignores directories that start with a period by default, but let me double check that. this may have been an accidental holdover from when i was getting the SDK up and running

[kanadgupta]

yeah looks like those directories aren't included by default, see microsoft/TypeScript#49555

[ilias-t]

lol

[ilias-t]

nit: the expect.assertions is a little confusing. Could we sub it with toHaveBeenCalledTimes?

[mjcuva]

cc @kanadgupta

[kanadgupta]

mock functions aren't applicable here, expect.assertions ensures that all the expect statements over here are being hit properly:

metrics-sdks/packages/node/test/index.test.ts

Lines 139 to 141 in dd18d50

	expect(body[0]._version).toBe(3);
	expect(body[0].group).toStrictEqual(outgoingGroup);
	expect(typeof body[0].request.log.entries[0].startedDateTime).toBe('string');

this pattern isn't great and i'm curious if others that have been in the weeds with MSW have better ideas for asserting request header content (or if that's even necessary) // @erunion

[erunion]

doing expect() in here or having the MSW mock return a 500 status code with a specific error is probably the best way to handle these

[ilias-t]

just want to make sure I'm reading this right, but securityRequirements is an array of objects and then we reduce that to an array of all the keys on that array of objects? Does it matter that we may have duplicate entries of the same key?

[mjcuva]

@jamestclark added this but I'm struggling to find how it is used. It's possible this existed in the main codebase and we reused parts of it in the sdk? However I don't think we have the OAS file at any point so I'm not sure

[jamestclark]

Doesn't matter that we may have duplicates as we just loop through the array and find the first matching from the users apiKey.

And yeah it's not used directly by the metrics SDK. As the code is very similar to what we already had in the main codebase he idea here is to co-locate all the securityRequirements code here and have the main codebase use this once it's published.

[brendanluna]

Adding @azinder1 as a reviewer here so he has context on unified snippet work (this came up just now in Metrics Pod sync!)

[mjcuva]

@erunion We likely will need to update sdk-snippets, but we're updating the UI in the dashboard separate from this work so I think we can just do that in another pr!

[kanadgupta]

i'm curious about others' opinions on this but my understanding of JS conventions is that variable names should only be capitalized in the case of classes/constructors (e.g., new Class(), etc.) — thoughts on lowercasing it? what about calling this metrics or something like that?

[mjcuva]

Sentry was my inspo here and they don't do that! https://docs.sentry.io/platforms/node/#configure

[kanadgupta]

interesting! sentry's SDK always gave class-like energy with the whole Sentry.init constructor pattern and it's apparently using AsyncLocalStorage to do that (which i don't totally understand...)

another idea here — what if you made this a class with a constructor? your example would look mostly the same (arguably a little cleaner + more intuitive to someone that expects JS conventions, in my opinion) and that way you can store these values as properties rather than globally which feels a little safer:

import { ReadMe } from 'readmeio';
- const readme = ReadMe(key);
+ const readme = new ReadMe(key);
app.use(readme.express(() => {}));

[mjcuva]

I think it's fine the way it is! It mimics other popular sdks and I don't really want to refactor this anymore tbh. I think it's important we get something out and stop going back and forth on nuances of the design!

[erunion]

agreed that it's a little weird to have a function be cased this way when that's generally how classes are defined + used.

what other popular sdks have exports like this?

[gratcliff]

@kanadgupta I like the idea! It's definitely best practice from an initialization perspective, and would properly set expectations for consuming developers.

[mjcuva]

I think if it's fine for Sentry it's understandable enough for our use case. I'm gonna move forward with it as it is! This PR is already way too long lived and we need to call off the rewriting somewhere. If someone else wants to take over this work feel free!