Refactor clerk/backend #2389

nikosdouvlis · 2023-12-18T11:47:17Z

Description

Review by PR

Checklist

npm test runs as expected.
npm run build runs as expected.
(If applicable) JSDoc comments have been added or updated for any package exports
(If applicable) Documentation has been updated

Type of change

Packages affected

changeset-bot · 2023-12-18T11:47:21Z

🦋 Changeset detected

Latest commit: 99906da

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 0 packages

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

dimkl · 2023-12-18T12:40:35Z

packages/backend/src/tokens/clerkRequest.ts

+
+class ClerkRequest extends Request {
+  clerkUrl: URL;
+  cookies: Map<string, string>;


❓ could we make the clerkUrl and cookies read-only to avoid accidentally replacing them or expecting that updating them would affect our result.
I think that both ClerkRequest and AuthenticateContext should be read-only objects.

packages/eslint-config-custom/typescript.js

dimkl · 2023-12-18T12:44:07Z

packages/backend/src/util/__tests__/request.test.ts

-    test('is not CO with IPv6', assert => {
-      const originURL = new URL('http://[::1]');
-      const host = new URL('http://[::1]').host;
-      assert.false(checkCrossOrigin({ originURL, host }));


❓ @SokratisVidros did we remove the checkCrossOrigin (which was part of the interstitial) in the handshake?

FYI, I've found no references in the codebase

packages/backend/src/tokens/authStatus.ts

dimkl · 2023-12-18T12:57:01Z

packages/backend/src/tokens/authenticateContext.ts

dimkl · 2023-12-18T12:59:53Z

packages/backend/src/tokens/types.ts

🔧 I would suggest we keep types close to their usage and use types.ts for generic types related to specific package. Eg move RequestStateParams to tokens/authStatus.ts or tokens/request.ts

I had to extract these to a new file to avoid circular dependencies between authStatus <> request, but let me take another look and see if I can avoid this

I need these for authenticateRequest and AuthenticateContext right now. Since we will continue refactoring the types and structs in clerk/backend after v5 as part of flex work, shall we fix this then?

nikosdouvlis · 2023-12-18T14:11:26Z

packages/backend/src/tokens/authenticateContext.ts

@@ -0,0 +1,88 @@
+import { constants } from '../constants';


These classes will be moved to a different dir when I wrap up my work on clerkMiddleware

SokratisVidros · 2023-12-18T14:11:36Z

packages/backend/src/tokens/authenticateContext.ts

+ * is in a signed in or signed out state or if we need
+ * to perform a handshake.
+ */
+class AuthenticateContext {


This naming feels weird as it looks like a React Context. Do we even need a class at this point? This is a very nit-picky comment, so you can ignore it for now, but I wanted to raise the point that the abstraction might not help as much as intended.

@nikosdouvlis Will we just pass a Request object afterall?

BTW this is a great candidate for async local storage.

... that the abstraction might not help as much as intended.
BTW this is a great candidate for async local storage.

Let me reply to these two together, as my intention is for us to use AuthenticateContext or a similar struct with Async Local Storage, but only after we simplify type-handling in clerk/backend and we decide that the abstraction works for us

I will not refactor anything else to use AuthenticateContext as I've simplified the problematic types for now. Let's do a design session and talk more about it

SokratisVidros · 2023-12-18T14:12:35Z

packages/backend/src/tokens/clerkRequest.ts

+import { constants } from '../constants';
+
+export type WithClerkUrl<T> = T & {
+  /**


❓ Do we still need this in the handshake world?

We still need it for authenticateRequest and redirectToSignIn/protect from within clerkMiddleware and authMiddleware

brkalow

Nice cleanup

brkalow · 2023-12-18T21:31:38Z

packages/backend/src/tokens/clerkRequest.ts

+export const createClerkRequest = (...args: ConstructorParameters<typeof ClerkRequest>): ClerkRequest => {
+  return new ClerkRequest(...args);
+};


❓ What's the benefit of using a factory function here? If it's a straight pass-through to the constructor it feels unecessary.

It's a pattern I like but don't have strong opinions about. It's a way to export a class as type only while providing a factory so consumers can use ClerkRequest as a type but they are forced to initialize via createClerkRequest.

I find that this helps me better understand what is a type vs a constructor when going through code quickly, but as I said, no strong opinions here.

This is exported under /internals so we can iterate as much as we like as for now, it's a "private" API

brkalow · 2023-12-18T21:35:15Z

packages/backend/src/tokens/authenticateContext.ts

+
+interface AuthenticateContextInterface extends AuthenticateRequestOptions {
+  // header-based values
+  sessionTokenInHeader: string | undefined;


❓ why do we use string | undefined instead of ?: string

In general, the two variations are almost always interchangeable, with one semantic difference being that string | undefined would mean that the prop exists in the object but is undefined (aka "something" in obj would return true) while ?: string means that it might now exist at all ("something" in obj might return false)

Here, we're initializing the values and even if some of them will be undefined, they do exist on this. Extremely nitpicky difference here so happy to change if you prefer the alternative syntax.

brkalow · 2023-12-18T21:36:23Z

packages/backend/src/tokens/authenticateContext.ts

+  sessionToken: string | undefined;
+}
+
+interface AuthenticateContext extends AuthenticateContextInterface {}


❓ Why are we doing this?

We're merging the interface with the class so we don't have to redefine the interface's props in the class it self

Makes sense 👍. In that case, I believe we can get rid of the intermediary AuthenticateContextInterface type (TS Playground).

This should work:

interface AuthenticateContext extends AuthenticateRequestOptions { // all the properties defined in AuthenticateContextInterface above... } class AuthenticateContext { // ... }

packages/backend/src/tokens/__tests__/clerkRequest.test.ts

brkalow · 2023-12-18T21:43:22Z

packages/backend/src/tokens/types.ts

+type RequestStateParams = {
+  publishableKey?: string;
+  domain?: string;
+  isSatellite?: boolean;
+  proxyUrl?: string;
+  signInUrl?: string;
+  signUpUrl?: string;
+  afterSignInUrl?: string;
+  afterSignUpUrl?: string;
+};
+
+export type AuthenticateRequestOptions = RequestStateParams & VerifyTokenOptions;


Can we consolidate these two types, or are we using RequestStateParams independently somewhere?

Thanks for spotting that, done!

A class that extends the native Request class, adds cookies helpers and a normalised clerkUrl that is constructed by using the values found in req.headers so it is able to work reliably when the app is running behind a proxy server.

A class that collects all data required to authenticate a request

clerk-cookie added backend fastify labels Dec 18, 2023

nikosdouvlis mentioned this pull request Dec 18, 2023

Backend cleanup #2383

Closed

24 tasks

nikosdouvlis marked this pull request as ready for review December 18, 2023 12:14

dimkl approved these changes Dec 18, 2023

View reviewed changes

dimkl assigned nikosdouvlis Dec 18, 2023

nikosdouvlis commented Dec 18, 2023

View reviewed changes

SokratisVidros reviewed Dec 18, 2023

View reviewed changes

SokratisVidros approved these changes Dec 18, 2023

View reviewed changes

clerk-cookie added nextjs remix labels Dec 18, 2023

brkalow reviewed Dec 18, 2023

View reviewed changes

nikosdouvlis added 8 commits December 19, 2023 00:35

feat(backend): Drop unused checkCrossOrigin util

e174b7f

feat(backend): Drop createIsomorphicRequest

91ae304

feat(backend): Introduce AuthenticateContext

d0bb44b

A class that collects all data required to authenticate a request

feat(backend): Simplify authenticateRequest and types

48711fd

fix(backend): Fix tests

c34f12f

fix(backend): Export createClerkRequest under /internal

64b71c6

fix(nextjs): Use new backend utils

20f7b78

nikosdouvlis force-pushed the nikos/backend-refactor branch from c9a44ac to 700c7e5 Compare December 18, 2023 22:39

clerk-cookie added the sdk-node label Dec 18, 2023

fix(nextjs): Fix init of clerkRequest

99906da

nikosdouvlis force-pushed the nikos/backend-refactor branch from 700c7e5 to 99906da Compare December 18, 2023 23:06

nikosdouvlis merged commit ad7bc70 into main Dec 18, 2023
7 checks passed

nikosdouvlis deleted the nikos/backend-refactor branch December 18, 2023 23:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor clerk/backend #2389

Refactor clerk/backend #2389

nikosdouvlis commented Dec 18, 2023 •

edited

Loading

changeset-bot bot commented Dec 18, 2023 •

edited

Loading

dimkl Dec 18, 2023

nikosdouvlis Dec 18, 2023

dimkl Dec 18, 2023

nikosdouvlis Dec 18, 2023

dimkl Dec 18, 2023

dimkl Dec 18, 2023

nikosdouvlis Dec 18, 2023

SokratisVidros Dec 18, 2023

nikosdouvlis Dec 18, 2023

nikosdouvlis Dec 18, 2023

SokratisVidros Dec 18, 2023

SokratisVidros Dec 18, 2023

SokratisVidros Dec 18, 2023

nikosdouvlis Dec 18, 2023

SokratisVidros Dec 18, 2023

nikosdouvlis Dec 18, 2023

brkalow left a comment

brkalow Dec 18, 2023

nikosdouvlis Dec 18, 2023

brkalow Dec 18, 2023

nikosdouvlis Dec 18, 2023

brkalow Dec 18, 2023

nikosdouvlis Dec 18, 2023

brkalow Dec 18, 2023

brkalow Dec 18, 2023

nikosdouvlis Dec 18, 2023

Refactor clerk/backend #2389

Refactor clerk/backend #2389

Conversation

nikosdouvlis commented Dec 18, 2023 • edited Loading

Description

Checklist

Type of change

Packages affected

changeset-bot bot commented Dec 18, 2023 • edited Loading

🦋 Changeset detected

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

brkalow left a 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

nikosdouvlis commented Dec 18, 2023 •

edited

Loading

changeset-bot bot commented Dec 18, 2023 •

edited

Loading