Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mockCurrentUser does not type-check as expected with TypeScript's strict option #5261

Closed
pvenable opened this issue Apr 20, 2022 · 5 comments · Fixed by #5491
Closed

mockCurrentUser does not type-check as expected with TypeScript's strict option #5261

pvenable opened this issue Apr 20, 2022 · 5 comments · Fixed by #5491
Assignees

Comments

@pvenable
Copy link
Contributor

pvenable commented Apr 20, 2022

It's not clear to me whether Redwood aims to support TypeScript's strict option (or strictNullChecks in particular), but I encountered this issue while experimenting with converting an existing app to Redwood.

(Repro of the below if helpful: branch / relevant commit)

Given strict: true in web/tsconfig.json, and the following code in api/src/lib/auth.ts:

export type CurrentUser = { id: number; roles: string[] }

export const getCurrentUser = (): CurrentUser | null => {
  return { id: 42, roles: ['user'] }
}

The following test code (in web, e.g. in a component test file) demonstrates some typing issues with mockCurrentUser:

import { CurrentUser } from 'src/lib/auth'

describe('mockCurrentUser with strict: true in tsconfig', () => {
  it('demonstrates that mockCurrentUser(null) does not type-check', () => {
    // This doesn't type-check even though getCurrentUser is typed to allow null
    mockCurrentUser(null)

    // Neither does this
    const user = null
    mockCurrentUser(user)
  })

  it('demonstrates that mockCurrentUser with a properly typed object literal does not type-check', () => {
    // Argument of type '{ id: number; roles: string[]; }' is not assignable to parameter of type 'CurrentUser'.
    // Object literal may only specify known properties, and 'id' does not exist in type 'CurrentUser'.ts(2345)
    mockCurrentUser({ id: 42, roles: ['test'] })
  })

  it('demonstrates that mockCurrentUser with a pre-declared const type-checks', () => {
    // This type-checks
    const user1: CurrentUser = { id: 42, roles: ['test'] }
    mockCurrentUser(user1)

    // And so does this, even without the type annotation
    const user2 = { id: 42, roles: ['test'] }
    mockCurrentUser(user2)
  })
})
@jtoar jtoar added this to Triage Apr 20, 2022
@jtoar jtoar moved this to Needs triage in Triage Apr 20, 2022
@jtoar jtoar added this to Main May 5, 2022
@jtoar jtoar moved this to Backlog in Main May 5, 2022
@jtoar jtoar removed this from Triage May 8, 2022
@jtoar jtoar moved this from Backlog to Triage in Main May 8, 2022
@jtoar jtoar added this to Release May 9, 2022
@jtoar jtoar moved this to Todo in Release May 9, 2022
@jtoar jtoar removed this from Release May 10, 2022
@jtoar jtoar moved this from Triage to In Progress in Main May 10, 2022
@dac09
Copy link
Contributor

dac09 commented May 17, 2022

Hi @pvenable - thank you for raising another great issue.

I've looked at this as part of strict mode support, but I believe the behaviour is as expected. Let's break it down:

1. Setting mockCurrentUser to null
CurrentUser can indeed be null, for example, when you're not signed in, or the auth state is still loading, so the type there is correct.

2. Setting mockCurrentUser with a roles array
This is the main one that could be improved, but I don't see a solution for this right now.

By default the currentUser.roles property is typed as string | string[]. This is because depending on the database you're using, string[] might not be possible for you to return from sqlite, for example. We also ship the auth templates with code for handling roles, and should we take the roles type out, there would be errors (even in on-strict mode)

Unfortuantely TypeScript does not let you "override" interfaces, but simply merge them. So what happens is it merges these two things:

// default Redwood interface for current user
export interface CurrentUser {
  roles?: Array<string> | string
}

// and once in your project, it gets merged with (see .redwood/types/includes/all-currentUser.d.ts, in your project)
// this is the type from src/lib/auth#getCurrentUser
interface ProjectCurrentUser {
 id: number,
 roles: string
}

// The result of which is
interface MergedCurrentUser {
 id: number,
 roles: string | string | Array<string>
}

I'm watching this issue over on the TypeScript repo: microsoft/TypeScript#36146 - but would be happy to hear if you have any other suggestions.

3. Importing CurrentUser type explicitly
mockCurrentUser is automatically typed by the generated types, so you shouldn't need to import CurrentUser from src/lib/auth - but you may have to make sure the types are updated after changes to your getCurrentUser - either by having the rw dev server running, or rw g types.


Very open to your feedback and suggestions - otherwise please do close the issue if there's nothing more to add here!

@pvenable
Copy link
Contributor Author

Hi, @dac09 -- thanks for your response.

CurrentUser can indeed be null, for example, when you're not signed in, or the auth state is still loading, so the type there is correct.

I completely agree that CurrentUser can be null. The issue I'm reporting here is that mockCurrentUser() does not accept null in strict mode -- passing null results in a type error like Argument of type 'null' is not assignable to parameter of type 'CurrentUser'. ts(2345).

I am short on time right now so I will digest the rest of what you have written later when I have a chance, but I do still believe there is a problem when trying to pass properties other than roles to mockCurrentUser as I have described above.
In my example, TypeScript is not happy with the inclusion of the id property in mockCurrentUser({ id: 42, roles: ['test'] }). Perhaps this is somehow intended and I'm misunderstanding? But it doesn't seem intuitive that I shouldn't be able to pass other properties. Or maybe what you're saying here is that it can't be helped due to the way interface merging works? Apologies if I'm being dense here. 🙂

Generating types didn't seem to address my issues either, but I will have to investigate more later.

Thanks!

@pvenable
Copy link
Contributor Author

After looking at this a bit more I must admit I remain stumped. I'm willing to change my api/src/lib/auth.ts code if it will help, but I haven't been able to find any combination of explicit or implicit typing for getCurrentUser and/or CurrentUser that will allow mockCurrentUser(null) to compile with strict: true on the web side. I don't see how this can be considered to be working as expected -- unless we're perhaps never meant to call mockCurrentUser(null) on the web side? Is that what I'm missing?

For the second issue (mockCurrentUser({ id: 42, roles: ['test'] })), I have found that as long as getCurrentUser cannot return null (either implicitly or explicitly), this example will compile. This seems odd to me, but I guess it means that everything I'm reporting here relates to the nullability of getCurrentUser.

@dac09
Copy link
Contributor

dac09 commented Jun 22, 2022

Hi Paul,

Sorry for the radio silence on this - had so many competing priorities.

So, I think you're right there is some strange behaviour that happens with the types - I'm stumped too. But I think I may have a solution.

mockCurrentUser get's it's type from the generated .redwood/types/includes/web-test-globals.d.ts file, where it tries to type the currentUser from '@redwoodjs/auth'. which should work in theory, but sometimes behaves oddly, especially in strict mode.

So I'm going to change the generated type file like this:

- import type { CurrentUser } from '@redwoodjs/auth'
+ import { InferredCurrentUser } from './all-currentUser'

declare global {
-   const mockCurrentUser: (currentUser: CurrentUser) => void
+  const mockCurrentUser: (currentUser: InferredCurrentUser) => void
}

You can try this out in your project as well, hopefully should solve your queries here.

@dac09 dac09 moved this from In progress to Needs review in Main Jun 28, 2022
@redwoodjs-bot redwoodjs-bot bot removed this from Main Jul 1, 2022
@zygopleural
Copy link
Contributor

@dac09 I'm still seeing this issue BTW with latest (3.2.0)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants