[RFC]: Harmonize and extend code completion hints on `currentUser`-object on web & api side

Summary

The basic idea here is to improve the DX revolving around auth.

As a developer working on my user model and writing tests for it I would like meaningful code completion on the currentUser-object and a flexible / resilient type system that meets the following requirements:

inform me where this object is created if I want to change it – i.e. give me a hint that I need to look into api/src/lib/auth/getCurrentUser in order to change it (including a JsDoc `{@Link …} so I can go there in one click) – s.th. like this:
when adding or removing optional fields to the return of getCurrentUser, existing code must not become invalid (yarn rw type-check will not show errors that were not there before the change)
this information on currentUser as listed above is uniform in both api-sides' context and the object provided by useAuth on the web-side

An additional bonus requirement what may be hinting towards a solution:

(as an extension to requirement 1.) have any fields listed on CurrentUser that getCurrentIUser is not returning yet. These fields map to my schema or my SDL (unsure yet which one, but I guess schema comes first) and are automatically updated when I update the specification of my user object there.

Motivation

Regarding 1.

Obvious. Make it easier for newbies to navigate and customize the auth-handling. Basically auth on rails. :steam_locomotive: ;)

Regarding 2.

This is a currently a real pitfall (unfortunately not into the pit of success, but this RFC aims to change that):
When you add an – albeit optional – field to the result of getCurrentUser (e.g. name), any parts of code involving a CurrentUser-object become red in VSCode and yarn rw type-check will suddenly start listing errors on each occurrence.

For me, such a change invalidates dozens of service tests (potentially hundreds) – adding a field will make yarn rw type-check spit out:

src/services/foobar/foobar.test.ts:352:23 - error TS2345: Argument of type '{ id: number; email: string; }' is not assignable to parameter of type 'CurrentUser'.
  Property 'name' is missing in type '{ id: number; email: string; }' but required in type 'CurrentUser'.

...for each of these occurrences. Now imagine you go ahead and change each and every one of these occurrences. At some point in the future, the decision is made to remove that field again (just from getCurrentUser – but it's still defined in the SDL and schema and thus a perfectly valid field!) and you'll get:

src/services/foobar/foobar.test.ts:354:9 - error TS2345: Argument of type '{ id: number; email: string; name: string}' is not assignable to parameter of type 'CurrentUser'.
  Object literal may only specify known properties, and 'name' does not exist in type 'CurrentUser'.

354         name: 'Chuck Norris',
            ~~~~~~~~~~~~~~~~~~~~~~~~~

I know there are may devs that would argue that the functionality of a project is unimpacted by these rather esoterical typescript errors, and that one should just learn to turn a blind eye on it and get on with their lives, but that's not how it works for many others.

TS-Newbies may be intimidated by these errors, seeing their code go red in various places just because they changed one line somewhere else, and I believe it's possible to come up with a more resilient type-system to avoid that. Also, there likely are projects that have the type-check as part of their CI/CD, so this has the potential to severely slow down time-to-market for RedwoodJS projects.

Regarding 3.

Currently, there seems to be a disjoint between api and the web side. On the API side, CurrentUser resolves to .redwood/types/includes/all-currentUser.d.ts, while on the web side the type of the same name resolves to node_modules/@redwoodjs/auth/dist/AuthProvider.d.ts.

The latter provides an additional roles?: Array<string> | string property and otherwise syncs with getCurrentUser (although I'm puzzled how it does that from looking at the implementation).

The former now also will have an optional roles property when #5856 is merged, but at the end of the day from a devs perspective I don't see why currentUser shouldn't be uniform on both sides.

Regarding 4.

Functionally, there is of course no functional gain in this, but it may be a helpful guidance and an important heads up which fields are not yet available on the currentUser-object (but recognizing they are now part of the user model) and giving a hint, similar to requirement 1. , where you'll need to go make it available.

Detailed proposal

(not very detailed though, additions welcome)

Regarding 1.

This is already implemented and will become available once #5856 is merged (also fixes the typescript errors and enables to go to getCurrentUser with a single click anywhere in the api side). It's included explicit requirement, however to ensure doesn't degrade or get lost during the refactorings required for the following requirements.

Regarding 2.

Not sure yet. But there must be a way to make non-essential fields in InferredCurrentUser optional. Some typescript guru out there will know, request for comments welcome.

Regarding 3.

Not exactly sure yet, but part of that already seems to be implemented, as outlined above. The currentUser returned from the useAuth-hook already is picking up the return values inferred from somewhere, it only needs to stop overwriting the roles-definition.

Regarding 4.

Basically we'd could look at something like Omit<Prisma.UserUncheckedCreateInput, 'hashedPassword' | 'salt'> and generate an interface with nice JSDoc from that (let's call that HelpfulUnimplementedAndCommentedProps for the sake of this explanation). Then have CurrentUser be Overwrite<HelpfulUnimplementedAndCommentedProps, InferredCurrentUser> (see Overwrite method here).

I have noticed a typescript error in .redwood/types/includes/api-globalContext.d.ts which may or may not be related to requirement no. 3. :

grafik

Nevertheless it may be worth looking into while getting our hands dirty on this RFC.

With all the trouble we're going through in both https://github.com/redwoodjs/redwood/pull/5856 and in this RFC I have a different idea:

What about not including any of the roles stuff at all by default?

I think I've heard the argument "But everyone is going to need RBAC sooner rather than later anyways, so might as well include it from the get-go". But I want to challenge that with personal experience. I have two RW apps I've been working on, on and off, for about two years now. Neither of them has any RBAC stuff yet.

Another point is that auth.ts is very overwhelming for new users right now with all the generated code and the many lines of comments. A lot of that complexity would go away if we didn't have the RBAC support in there.

So my suggestion is:

Generate auth without roles by default
Add a --roles or --rbac flag to yarn rw setup auth to include the roles stuff for those who know they'll need it
Make it possible to run yarn rw setup auth --roles (or some other command) on a project that's already setup with auth to add roles support after the fact. This is the most difficult part because it has to take into consideration that the user might have already made modifications to the generated auth related files, and we don't want the user to lose those changes.
Add a (short) comment to auth.ts educating our users about the fact that they can run a command to add roles support if they need it
Add a comment to the end of the yarn rw setup auth output educating our users about the fact that they can run a command to add roles support if they need it in the future
Make roles required (non-optional) everywhere when added by the new command
Make sure we select roles from the database when finding the user when adding roles suport with the new command

(This is just my personal thoughts that I just came up with. Not the view of the RW core team (yet 😜). Please do tell me about all the ways this is a horrible idea)

Great writeup @Philzen 👏

This is my first post on Redwood, I'm super excited about this project and still learning the ropes, only on Chapter 4 so far. Nice to meet you all 👋

I stumbled upon this RFC tonight while running through tutorial Chapter4#add-a-logout-link and have comments about the typings issue on point No.3 if I may:

I definitely was one of those people who thought: "Oh I must have done something wrong here, the email type is missing from the current user. Silly me.". If I was an impatient, entitled son-of-a-gun, this could be the point of the tutorial where I throw in the towel and declare my interest in RedwoodJS as 'expired'. Not me though, I'm still keen 💚. I 100% agree with the idea of improving of DX around auth, definitely one worth pursuing imho.
When availing of useAuth() and seeing the type error [ts] Property 'email' does not exist on type 'CurrentUser'., I instinctively checked to see if useAuth simply accepted a generic type, so I could manually just override currentUser and move on with the tutorial which I was enjoying so much. So I thought I would try this out of curiosity:
```
const { isAuthenticated, getCurrentUser, logOut } = useAuth<{
name?: string | null;
email: string;
hashedPassword: string;
salt: string;
resetToken?: string | null;
resetTokenExpiresAt?: Date | string | null;
}>();
```
❌ Obviously this didn't work, useAuth does not take a generic. But I'm just noting here that this was a thought process of mine to see if I could escape from the situation and get on with Chapter 4+. Ideally the currentUser type would be auto-inferred as previously discussed, but in the event I wanted to add something custom, popping a generic before useAuth feels like a natural thing to do, were it to be available.

For those of you playing at home and stuck with the TS error for now, I ended up using this as a makeshift type casting workaround to make the TS gods less angry and more cuddly:

import { Prisma } from '.prisma/client/index';
...
// in the component:
const { isAuthenticated, logOut } = useAuth();
const currentUser = useAuth().currentUser as unknown as Prisma.UserCreateInput;

I noted that the type for currentUser 🔴 on the useAuth hook currently comes from AuthContextInterface in node_modules/@redwoodjs/auth/dist/AuthProvider.d.ts

export interface AuthContextInterface {
loading: boolean;
isAuthenticated: boolean;
currentUser: null | CurrentUser; 🔴
userMetadata: null | SupportedUserMetadata;
logIn(options?: unknown): Promise<any>;
// ....and so on
}

Total n00b question here but would it be a good idea to update this 🟢 to:

export interface AuthContextInterface {
loading: boolean;
isAuthenticated: boolean;
currentUser: null | Prisma.UserCreateInput; 🟢
userMetadata: null | SupportedUserMetadata;
logIn(options?: unknown): Promise<any>;
// ....and so on
}

No idea if this is good/bad/smart/dumb but just asking the question. I am not sure if Prisma.UserCreateInput is static, or is auto-generated from the User model in api/db/schema.prisma?

Thank you all for the work already put into this amazing project ☘️

I definitely was one of those people who thought: "Oh I must have done something wrong here, the email type is missing from the current user. Silly me." [...] I 100% agree with the idea of improving of DX around auth, definitely one worth pursuing imho.

Thanks for validating our efforts!

@dac09 I'm keen to get your input here. Thanks!

I don’t really want to engage on the other topics just yet - but happy that you guys are!

But

I like the idea of the useAuth generic although…
The type of currentUser in useAuth comes from the getCurrentUser function in src/lib/auth, there really should be no need to directly use the prisma types. @noccer did you return the email field from that function?

currentUser: null | Prisma.UserCreateInput; 🟢

This isn't a good idea I'm afraid, its too specific to your project perhaps - and also a bit odd that we'd use the CreateInput type here. Remember when writing these types in the framework:

a) You cannot assume everyone uses dbAuth, and even returns a Prisma type b) You cannot assume that everyone using dbAuth uses a model called "User" c) You cannot assume that they will return the user object at all, maybe they return something else entirely from the getCurrentUser function.

When something goes red because you’re accessing a property that you aren’t returning from that function, it’s a good thing. Why would you want typechecking to pass when that property doesn’t exist? Or did I misunderstand @Philzen?

when adding or removing optional fields to the return of getCurrentUser, existing code must not become invalid (yarn rw type-check will not show errors that were not there before the change)

This is entirely the point of the types no? That if you change the shape of currentUser, and downstream something else expects the old shape, theres an error.

The type of roles never gets fully overridden in useAuth.currentUser, and that’s unfortunately how typescript does declaration merging. There is no way to fully override when you merge interfaces (there’s an open issue somewhere in the TS repo). I wonder if there’s some way we can use the Overwrite generic here, instead of merging interfaces.

redwoodjs / redwood