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

Remove nullish types in GlobalContext, add 'loading' variable #31

Open
Xevion opened this issue Sep 20, 2023 · 0 comments
Open

Remove nullish types in GlobalContext, add 'loading' variable #31

Xevion opened this issue Sep 20, 2023 · 0 comments
Labels
rework Something existing needs to be redeveloped or modified

Comments

@Xevion
Copy link
Member

Xevion commented Sep 20, 2023

          So the concept I was going for was eliminating the 'nullish' portion of these types, making them straight booleans. Then, checking whether we've finished loading can be done with a separate, third variable, `loading`. The `admin` and `member` properties can be `false` by default, assuming an unauthenticated state.

The reason why we're concerned with loading is in the NavBar when we're still figuring out if a user is authenticated, and changing whether to show 'Login' or 'Register'. That flash of unauthenticated content, or whatever you want to call it, is confusing.

Like, what would you think if you logged onto a website and it said 'Login' and then 0.5 seconds later it flashes to "Hello, {name}"? That's the purpose of the skeleton components (the loading indicator).

That said, the current types bother me; we're already executing two separate routes - login checks for both the member and admin sides, every single time. Even normal users do it. And while tRPC is nice and bundles them together - I think we could do with a singular login route and abstracting the loading logic to a separate variable.

Pros:

  • Single route, better performance
  • Get rid of nullish values
  • More explicit meaning of what is being checked with 'loading'
    • We could do "loadingAuthentication" or "loadingContext" or something else, but the other variables are about the same in terms of naming & understanding. Null values have no explicit meaning.

Cons:

  • Can no longer have individual loading states for admin & member authentication - in the global context. Not much of a loss, really.
  • Minor refactor. Typescript should raise all the usage points, so it shouldn't be hard to find where to refactor. Search or find usages & whatever.

Originally posted by @Xevion in #27 (comment)

@Xevion Xevion added the rework Something existing needs to be redeveloped or modified label Sep 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rework Something existing needs to be redeveloped or modified
Projects
None yet
Development

No branches or pull requests

1 participant