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

feat: add useAuth composable and discriminated union for $auth #28

Merged
merged 16 commits into from Dec 6, 2023

Conversation

GioPat
Copy link
Contributor

@GioPat GioPat commented Dec 3, 2023

Issue
When using strinct type checking $auth is not found.

Solution

  • Add explicit composable and type
  • Convert anonymous types to explicit

package.json Outdated
@@ -1,6 +1,6 @@
{
"name": "@nuxtjs/kinde",
"version": "0.1.3",
"version": "0.1.4",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Even though this should be a minor release based on semver 2.0

@danielroe
Copy link
Collaborator

Would you provide a reproduction of the bug? 🙏

package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
src/runtime/types.ts Outdated Show resolved Hide resolved
src/runtime/types.ts Outdated Show resolved Hide resolved
src/module.ts Outdated Show resolved Hide resolved
@GioPat
Copy link
Contributor Author

GioPat commented Dec 4, 2023

Would you provide a reproduction of the bug? 🙏

Simply install the module with strict typescript settings and won't find the $auth state keyword.
I think it's missing the typing extension of the Vue state: here you can find the source code of a working example that is "fixing" this behavior.
In any case, I added the explicit composable to avoid any issues.
This is the doc to the official nuxt doc: https://nuxt.com/docs/guide/directory-structure/plugins#typing-plugins

@GioPat
Copy link
Contributor Author

GioPat commented Dec 4, 2023

I think it's also worth to update the README with the newly added composable.

Comment on lines 3 to 6
export type AuthState = {
loggedIn: boolean
user: UserType | null
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I split this out in my PR also, but this type I feel is a little too open, it will allow false with a userType and true with no userType

Copy link
Contributor Author

@GioPat GioPat Dec 4, 2023

Choose a reason for hiding this comment

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

Are you also solving the issue of the typing ? Adding a composable? If so for me it's ok to close this PR in favour of yours.
If we restrict the type typescript is gonna complain about "Cannot assing boolean to false" But I'll try to do it! Thanks for the warning

Copy link
Collaborator

Choose a reason for hiding this comment

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

We could totally combine the PRs, my ones exposes the kinde client so can access things like permissions, feature flags etc.

I did add a composable at one stage but it was out of scope of the change I was making was going to add later.

I get your concern about the boolean error, it would only complain about setting the boolean to false. I cannot think of a scenario where you would want false with a user. Can you think of one?

Copy link
Collaborator

Choose a reason for hiding this comment

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

PS: this is the PR I was referring to: #27

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Typing restricted, I don't know why the commit is not reflected.
If you want feel free to take my code in your PR and we close this one 🙂

@GioPat GioPat changed the base branch from main to renovate/lock-file-maintenance December 5, 2023 20:19
Copy link
Collaborator

@danielroe danielroe left a comment

Choose a reason for hiding this comment

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

This looks good to me.

I'll fix issue with untyped $auth separately.

@danielroe danielroe changed the title Add composable to explicitly use auth object feat: add useAuth composable and discriminated union for $auth Dec 6, 2023
@danielroe danielroe merged commit 3a2a560 into nuxt-modules:renovate/lock-file-maintenance Dec 6, 2023
@danielroe
Copy link
Collaborator

just realised this wasn't opened against main; cherry-picked it in via f1f04c7.

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 this pull request may close these issues.

None yet

3 participants