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

Add generic Userinfo to createFusionAuth for vue sdk #143

Merged
merged 6 commits into from
Jun 25, 2024

Conversation

michaelyons
Copy link
Collaborator

@michaelyons michaelyons commented Jun 24, 2024

What is this PR and why do we need it?

#139 -- adding an optional generic type to createFusionAuth so that UserInfo can be custom typed. Defaults to UserInfo

Pre-Merge Checklist (if applicable)

  • Unit and Feature tests have been added/updated for logic changes, or there is a justifiable reason for not doing so.

@michaelyons michaelyons linked an issue Jun 24, 2024 that may be closed by this pull request
Copy link
Collaborator

@JakeLo123 JakeLo123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need the option to specify the type in conjunction with createFusionAuth and useFusionAuth. So we need the end result to look something like:

type Whatever = {
  // ...etc
};
const { userInfo } = createFusionAuth<Whatever>(config);
const { userInfo } = useFusionAuth<Whatever>();
// Where `userInfo` is inferred to be of type `Whatever`.

We will need to update the following definitions:

  • FusionAuth to accept an optional generic where userInfo and getUserInfo reference that T type.
  • createFusionAuth to accept the same optional generic argument where userInfo and getUserInfo can reference T.
  • useFusionAuth to accept the same so it can return userInfo and getUserInfo as the generic passed in.

If it's done correctly, you should be able to consume a local build in the quickstart and get strong typing for a custom userinfo type you pass in.

Please let me know if this is helpful or needs clarifying! Also happy to pair, since I've already done this for the other frameworks, it could go pretty fast.

@JakeLo123 JakeLo123 self-requested a review June 24, 2024 21:08
Copy link
Collaborator

@JakeLo123 JakeLo123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this won't build.

image

@JakeLo123 JakeLo123 requested a review from BriDavidson June 24, 2024 21:12

export const useFusionAuth = (): FusionAuth => {
export const useFusionAuth = <T extends FusionAuth<UserInfo>>(): FusionAuth => {
Copy link
Collaborator

@JakeLo123 JakeLo123 Jun 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps try <T = UserInfo>(): FusionAuth<T> here. That would define T as the generic, with UserInfo being the default value for T if none is provided. Then make sure to pass it along to the FusionAuth return type. That's what will give it the strong typing!

const user = {
given_name: 'JSON',
family_name: 'Bourne',
customTrait: 'additional info',
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason for this change? I believe we can use this test to cover what we're implementing here, but I'm not sure this is doing it. Here's an example

import { SDKCore } from '@fusionauth-sdk/core';
import { FusionAuth, FusionAuthConfig, UserInfo } from '#/types';

import { NuxtUseCookieAdapter } from './NuxtUseCookieAdapter';

export const createFusionAuth = (config: FusionAuthConfig): FusionAuth => {
export const createFusionAuth = <T extends UserInfo>(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We want <T = UserInfo> not extends. I haven't used extends much but I believe this would mean T is constrained to have all the properties defined on UserInfo, which is not what we want.

@@ -19,16 +21,16 @@ export const createFusionAuth = (config: FusionAuthConfig): FusionAuth => {
});

const isLoggedIn = ref<boolean>(core.isLoggedIn);
const userInfo = ref<UserInfo | null>(null);
const userInfo = ref<T | null>(null) as Ref<UserInfo>;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we please use const userInfo Ref<T | null> = ref(null)?
This thread has an answer from a vue contributor explaining that ref<>() with generic types causes issues.

@JakeLo123 JakeLo123 self-requested a review June 25, 2024 15:57
Copy link
Collaborator

@JakeLo123 JakeLo123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We get generic typing in our nuxt plugin as well!
image
image

import { FusionAuth, FusionAuthConfig } from './types.ts';
import * as components from './components/index.ts';
import { fusionAuthKey } from './injectionSymbols.ts';
import { createFusionAuth } from './createFusionAuth/index.ts';
import { UserInfo } from '@fusionauth-sdk/core';
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be UserInfo from ./types.ts

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#147
Separate issue, though. Here's a ticket #147

@JakeLo123 JakeLo123 merged commit ce690f3 into main Jun 25, 2024
3 checks passed
@JakeLo123 JakeLo123 deleted the 139-vue-typedgeneric-userinfo branch June 25, 2024 16:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Vue] Typed/generic UserInfo
3 participants