Refactor: separate authMachine from React #5110
Conversation
|
QA Wolf here! As you write new code it's important that your test coverage is keeping up. |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5110 +/- ##
=======================================
Coverage 86.14% 86.14%
=======================================
Files 89 89
Lines 32489 32489
=======================================
Hits 27987 27987
Misses 4502 4502
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
franknoirot
force-pushed
the
franknoirot/adhoc/project-file-out-of-react
branch
from
249e85e to
928b8ab
Compare
|
I think your other settings PR includes code from here :) |
|
|
||
| /** | ||
| * A simple HOC that listens to the auth state of the app and navigates | ||
| * accordingly. | ||
| */ | ||
| export function AuthNavigationHandler({ |
There was a problem hiding this comment.
No way we could make this a hook eh? I see how you're using it but I believe it could be used in the Router component. It feels weird to use this as a HOC when it's not modifying the component(s) at all.
There was a problem hiding this comment.
You're absolutely right, I should have made this a hook from the jump. I've just made that change.
| export const useAuthState = () => useSelector(authActor, (state) => state) | ||
| export const useToken = () => | ||
| useSelector(authActor, (state) => state.context.token) | ||
| export const useUser = () => | ||
| useSelector(authActor, (state) => state.context.user) |
There was a problem hiding this comment.
I'm personally not a fan of these very shallow uses of useSelector, but up to you :). It hides that useToken and useUser are just simple useSelector 's underneath.
There was a problem hiding this comment.
Hm I don't know, I'm trying to make a very simple DX for teammates to use the token in the future. I heard that you should define your selector closures outside of React anyway (here in a comment in an example in the XState docs), and I didn't want every developer to have to remember to do that. What do you think about that, does hiding that detail seem worthwhile?
There was a problem hiding this comment.
That XState comment is only talking about the selector function but not useSelector itself (you see they're all inside a React component).
I believe it makes sense to abstract the selector (the developer doesn't need to know the details of what's being selected in fine detail), but not the useSelector hook itself (which dictates a lot of behavior and probably don't want to "clutter" a developer's selection of hooks to chose from with 101 hooks that are all essentially useSelector)
| export const authActor = appActor.system.get(ACTOR_IDS.AUTH) as ActorRefFrom< | ||
| typeof authMachine | ||
| > |
There was a problem hiding this comment.
TypeScript can't infer the type properly I take it?
There was a problem hiding this comment.
No, this is my biggest beef so far with trying to make use of the XState actor system approach. Maybe I'm missing something but appActor.system.get() has this type signature according to my intellisense:
(property) ActorSystem<any>.get: <string>(key: string) => any
There was a problem hiding this comment.
Yeah we'll look together at a better solution - not worth blocking IMO.
| export const ACTOR_IDS = { | ||
| AUTH: 'auth', | ||
| } |
There was a problem hiding this comment.
I would rather each Actor own its id, instead of declaring them all here. If we want to export a full list we can import them into one place but I want each actor to dictate its own information.
There was a problem hiding this comment.
But that information—the actor's src, id, and systemId—are all the same string, and we try to make any repetitive string into a named constant?
There was a problem hiding this comment.
Another goofy part of this is that in this first PR I invoked the authMachine within appMachine, but once I got started adding settingsMachine in the next PR I realized I should spawnChild them.
There was a problem hiding this comment.
Oh yea, my gripe is the location of the information. I believe it should be more local to the actor, instead of separated out like this :) Just my opinion of course - there is no absolute right.
|
|
||
| async function logout() { | ||
| localStorage.removeItem(TOKEN_PERSIST_KEY) | ||
| if (isDesktop()) return Promise.resolve(null) |
There was a problem hiding this comment.
User's can't logout on desktop? 🤔
franknoirot
force-pushed
the
franknoirot/adhoc/project-file-out-of-react
branch
from
928b8ab to
019cc7f
Compare
Continuation of the work to separate out XState actors from React wherever possible.
This PR also begins the use of an XState system, which subsequent PRs will continue to use.