[franciscop]

Type of change

• 🐛 Bug fix
• 🌟 New feature
• 🔨 Breaking change
• 📖 Refactoring / dependency upgrade / documentation
• other:

Packages affected

• @clerk/clerk-js
• @clerk/clerk-react
• @clerk/nextjs
• @clerk/remix
• @clerk/types
• @clerk/themes
• @clerk/clerk-expo
• @clerk/backend-core
• @clerk/clerk-sdk-node
• @clerk/edge
• build/tooling/chore

Description

• npm test runs as expected.
• npm run build runs as expected.

Following on this issue #201, made the functions returned by useAuth() stable (as per the usual React terminology); meaning they will not change between unrelated re-renders. This allows you to properly depend on them without worries about re-renders, and when the underlying value does change (isomorphicClerk), then they will update properly.

The logic inside useAuth() should be identical as before, just reorganized it a bit to be able to use a useMemo to make auth also stable.

PS, modified the types a bit, unfortunately I'm not that familiar with TS but the default installation was giving me types issues (before any code change, just by importing the files into the test file). So some of the changes were fixes, and the one modifying UseAuthReturn was because I had trouble with getting the highly-dynamic variable to work otherwise.

#201

[SokratisVidros]

Thanks for the taking a stab at it. I've just added my comments after the first review.

[franciscop]

Thanks for the feedback! I'm in my lunch break, I won't be able to commit anything until the weekend, so feel free to add any other comment and I'll try to fix them all then.

[franciscop]

Fixed the given points! Unfortunately I still had to change the types into 3 parts and cast each return independently with as {NAME}, since otherwise with the default code I was getting this error which I couldn't figure out how to fix in any other way:

src/hooks/useAuth.ts:80:14 - error TS2322: Type '() => { isLoaded: false; isSignedIn: any; sessionId: string; userId: string; actor: any; signOut: SignOut; getToken: GetToken; } | { isLoaded: true; isSignedIn: false; sessionId: string; userId: string; actor: any; signOut: SignOut; getToken: GetToken; } | { ...; }' is not assignable to type 'UseAuth'.
@clerk/clerk-react:test:       Type '{ isLoaded: false; isSignedIn: any; sessionId: string; userId: string; actor: any; signOut: SignOut; getToken: GetToken; } | { isLoaded: true; isSignedIn: false; sessionId: string; userId: string; actor: any; signOut: SignOut; getToken: GetToken; } | { ...; }' is not assignable to type 'UseAuthReturn'.
@clerk/clerk-react:test:         Type '{ isLoaded: false; isSignedIn: any; sessionId: string; userId: string; actor: any; signOut: SignOut; getToken: GetToken; }' is not assignable to type 'UseAuthReturn'.
@clerk/clerk-react:test:           Type '{ isLoaded: false; isSignedIn: any; sessionId: string; userId: string; actor: any; signOut: SignOut; getToken: GetToken; }' is not assignable to type '{ isLoaded: true; isSignedIn: true; userId: string; sessionId: string; actor: ActJWTClaim; signOut: SignOut; getToken: GetToken; }'.
@clerk/clerk-react:test:             Types of property 'isLoaded' are incompatible.
@clerk/clerk-react:test:               Type 'false' is not assignable to type 'true'.
@clerk/clerk-react:test: 
@clerk/clerk-react:test:     80 export const useAuth: UseAuth = () => {

This error was not shown when running the tests without useAuth.test.tsx, which makes me think it's something visible either only when running the tests, or because of the mocking the clerk instance is not being done properly. I believe these type changes should be totally retro-compatible, but please let me know if those are not acceptable.

[SokratisVidros]

❓ Is this type assertion required?

[franciscop]

Without this, npm test will give me this issue:

src/contexts/ClerkContextProvider.tsx:149:9 - error TS2322: Type 'string | UserResource' is not assignable to type 'string'.
      Type 'UserResource' is not assignable to type 'string'.

    149   const userId: string | null | undefined = state.user ? state.user.id : state.user;

Alternatively these also work:

const userId: string | null | undefined = state.user && state.user.id;

[SokratisVidros]

🔧 This should also be an undefined. It was a mistake to make it null in this scenario in the first place.

[SokratisVidros]

A better naming for this interface is the UnknownState as undefined means that we can't tell yet if the user is signed in or signed out.

[SokratisVidros]

Let's avoid creating these three different states and stick to the original type definition to avoid the extra assertions.

[franciscop]

If I use the original, TS doesn't seem to differentiate them properly and I get this error:

Type '() => { isLoaded: false; isSignedIn: any; sessionId: any; userId: any; actor: any; signOut: SignOut; getToken: GetToken; } | { isLoaded: true; isSignedIn: false; sessionId: string; userId: string; actor: any; signOut: SignOut; getToken: GetToken; } | { ...; }' is not assignable to type 'UseAuth'.
      Type '{ isLoaded: false; isSignedIn: any; sessionId: any; userId: any; actor: any; signOut: SignOut; getToken: GetToken; } | { isLoaded: true; isSignedIn: false; sessionId: string; userId: string; actor: any; signOut: SignOut; getToken: GetToken; } | { ...; }' is not assignable to type 'UseAuthReturn'.
        Type '{ isLoaded: true; isSignedIn: false; sessionId: string; userId: string; actor: any; signOut: SignOut; getToken: GetToken; }' is not assignable to type 'UseAuthReturn'.
          Type '{ isLoaded: true; isSignedIn: false; sessionId: string; userId: string; actor: any; signOut: SignOut; getToken: GetToken; }' is not assignable to type '{ isLoaded: true; isSignedIn: true; userId: string; sessionId: string; actor: ActJWTClaim; signOut: SignOut; getToken: GetToken; }'.
            Types of property 'isSignedIn' are incompatible.
              Type 'false' is not assignable to type 'true'.

[dimkl]

@franciscop thank you for the contribution but we will close this PR in favor of : #980