-
-
Couldn't load subscription status.
- Fork 184
Make use session sync, use a plugin based approach for session fetching #69
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
Conversation
…igate utiltiy to urls.ts
…nt it, make useSession data read-only, improve docs by expanding getting started + configuration section + credit @JoaoPedroAS51 + fix type check command, add lastRefrehsedAt to app.vue, add a new example page to the playground that will always be unprotected, remove now() util in favor of a real date, make `getCsrfToken` return the token right away
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome!!! Nice work!! 😃
I just pointed out a thing here and there.
src/runtime/utils/url.ts
Outdated
| import { useRequestEvent } from '#app' | ||
| import { useRuntimeConfig, navigateTo as _navigateTo } from '#imports' | ||
|
|
||
| const _getBasePath = () => parseURL(useRuntimeConfig().public.auth.url).pathname |
There was a problem hiding this comment.
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 we use two different funcs to get the url? I'm just concerned about causing a mismatch between _getBasePath and getRequestUrl
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Originally, I did this to ensure that when the origin is set, it's strictly used on the app- and server-side. However, that doesn't make any sense as on the app-side anyone can edit the js/html/... anyways.
I'll use the useRequestEvent here but keep using the origin in the auth handler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can create a func that uses the origin defined by the user and if not defined fallback to requrl. Makes sense? I believe this would fix the host/port issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought on the server-side we need it for security reasons, but you may be right that we can drop it. Looking through the next auth docs and code I cannot find a place that requires it, only the secret / NEXTAUTH_SECRET.
Let's investige + remove this in another issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've opened #72 foer this (:
src/runtime/plugin.ts
Outdated
| const { data, lastRefreshedAt } = useSessionState() | ||
| const { getSession } = useSession() | ||
|
|
||
| await getSession() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we are not using Broadcast yet, we can add a check here to avoid an unnecessary request in client side if the session was already fetched in server side.
| await getSession() | |
| // Fetch the session only if not initialized yet. | |
| if (data.value === undefined) { | |
| await getSession() | |
| } |
src/runtime/middleware/auth.ts
Outdated
| return | ||
| } | ||
|
|
||
| await signIn(undefined, { callbackUrl: to.path }, { error: 'SessionRequired' }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can use return here instead of await
| status, | ||
| data | ||
| const getters = { | ||
| status: readonly(status), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As status is now computed, we don't need to add readonly
Closes #65
This PR:
useSessionsynclastRefreshedAtas a public propertyuseStateinto a single place to make it easier to maintain + updateStill to do:
for later work to update https://github.com/sidebase/nuxt-auth-example:
getCsrfTokenchangedfor new issues / out of scope:
useSessionStatecomposableChecklist:
#)