-
Notifications
You must be signed in to change notification settings - Fork 8
move auth to store and cleanup #127
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
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.
Good call 🔥 was a bit odd to have these two states separated from the store
const keys = useSelectorStore(store, (state) => state.context.keys); | ||
const identity: Identity.Identity | null = | ||
accountId && keys | ||
? { | ||
accountId, | ||
encryptionPublicKey: keys.encryptionPublicKey, | ||
encryptionPrivateKey: keys.encryptionPrivateKey, | ||
signaturePublicKey: keys.signaturePublicKey, | ||
signaturePrivateKey: keys.signaturePrivateKey, | ||
} | ||
: null; | ||
return identity; |
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.
Could you wrap this in the useSelectorStore
fn so its get memoized? Like
export function useHypergraphIdentity() {
return useSelectorStore(store, (state) => {
const accountId = state.context.accountId;
const keys = state.context.keys;
if (accountId && keys) {
return { accountId, ...keys }
}
return null
})
}
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.
oh! that's a great suggestion. will do, thx
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.
this actually doesn't work :(
I get:
The result of getSnapshot should be cached to avoid an infinite loop Error Component Stack
Error: Maximum update depth exceeded. This can happen when a component repeatedly calls setState inside componentWillUpdate or componentDidUpdate. React limits the number of nested updates to prevent infinite loops.
I guess the reason is because they are using useSyncExternalStore
under the hood and in this case the returned result would be a new object every time and referential equality never results in a true when comparing the previous and new one.
We could create a useRef to store the object, but not sure it's worth the trouble? What do you think? Any other suggestions?
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.
merged it to continue refactoring, but happy to incorporate any ideas in a new PR
c2050a8
to
bd468be
Compare
By moving auth to the store it's data is available in the core framework and not only React. This also will allows use as a next step to move a lot of the auth functions to the core. Follow up issue: #128
Details:
loginWithWallet
&loginWithKeys
where we could end up on an infinite loop. I added retry counts to prevent this and throw if it doesn't work after 3 times.I changed the
check if the user is already authenticated on initial render
in a useEffect to be a useLayoutEffect since this will execute the whole state change before anything is commited to the DOM and therefor prevent flickering.