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

Check session #172

Closed
wants to merge 2 commits into from
Closed

Check session #172

wants to merge 2 commits into from

Conversation

deo002
Copy link
Contributor

@deo002 deo002 commented Nov 17, 2023

No description provided.

@deo002
Copy link
Contributor Author

deo002 commented Nov 18, 2023

I'll add too other pages as well once this is approved

@hoangnt2
Copy link
Contributor

@deo002 I'm wondering where we can put it. It is a component but HOC is quite different in meaning.

@deo002
Copy link
Contributor Author

deo002 commented Nov 20, 2023

@deo002 I'm wondering where we can put it. It is a component but HOC is quite different in meaning.

Hmm...how about a folder named HOCs inside of components/sw360 and import it like this: import 'withAuth' from '@/components/sw360/HOCs'.

Also, I was considering renaming withAuth to loadSession.

What do you think?

@hoangnt2
Copy link
Contributor

hoangnt2 commented Nov 20, 2023

@deo002 I think hocs should be on same level with the components folder.

image

Here is example from ChatGPT

and withAuth should be kept (I had taken a look on some HOC example and they use that name format with + sth)

@heliocastro could you give me and @deo002 some advices?

@heliocastro
Copy link
Contributor

heliocastro commented Nov 20, 2023

I'm still not 100% convinced though.
Is higher order component the correct solution and / or the only ?

The way set by some of the nextjs auth docs is like we can protect the routes without a HOC, so i'm missing something.
Can we sit on this his week and look if there's no other alternatives to evaluate ?

And @deo002 be careful with gpt like tools, only consider any entry if you can read and explain what is done to yourself, otherwise is a no-go.

@hoangnt2
Copy link
Contributor

hoangnt2 commented Nov 20, 2023

@heliocastro

The routes are protected by middleware.ts (as we configured in the project).

The problem with useSession is when you use useSession, it will fetch the session from api/auth/session and it will take some time to wait so we need a piece of code to check if the session is fetched or not. I'm using HOC to reduce code duplication (check session code), not to protect routes.

@deo002
Copy link
Contributor Author

deo002 commented Nov 20, 2023

@heliocastro The hoc is not for protecting routes, it's sole purpose is to wait(show spinner) until the session info is loaded and then provide it to the components that use it.

If we use session before it has loaded, we get this error because session.user.token is undefined until then:
Screenshot from 2023-11-06 11-13-05

We can discuss alternative approaches or how to improve this approach this week.

@deo002
Copy link
Contributor Author

deo002 commented Nov 20, 2023

@deo002 I think hocs should be on same level with the components folder.

image

Here is example from ChatGPT

and withAuth should be kept (I had taken a look on some HOC example and they use that name format with + sth)

@heliocastro could you give me and @deo002 some advices?

Sounds good!

@heliocastro
Copy link
Contributor

We can discuss alternative approaches or how to improve this approach this week.

useSession has a intermediate state ( loading ), to do exactly the timing difference. We do need discuss this one, before we overcomplicate all.

@deo002
Copy link
Contributor Author

deo002 commented Nov 20, 2023

We can discuss alternative approaches or how to improve this approach this week.

useSession has a intermediate state ( loading ), to do exactly the timing difference. We do need discuss this one, before we overcomplicate all.

Sure! Let's discuss.

@heliocastro
Copy link
Contributor

Closing this one as we are already using better solution

@heliocastro heliocastro closed this May 6, 2024
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.

3 participants