-
Notifications
You must be signed in to change notification settings - Fork 72
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
Reduce dependency on the VM #1287
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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.
Thanks for taking this on @gagdiez! One thing I'm hesitant on with this refactoring is the confusion it creates for knowing when to use useAuthStore()
vs useContext(NearContext)
. I'm pretty familiar with this codebase and I have to admit that I'm already a bit confused.
I might suggest picking one of these routes to continue the refactor before merging:
- Completely remove
stores/auth.ts
and refactor away all uses ofuseAuthStore()
. - Rework your initialization logic to store state in
stores/auth.ts
so that we can continue with our familiar pattern ofuseAuthStore()
- in this way we wouldn't be callinguseContext(NearContext)
directly and would probably deleteNearContext
all together.
I would probably lean towards option 2 since Zustand tends to make our interactions more consistent as complexities grow.
What do you think @shelegdmitriy?
I like the usage of |
Sure, I can go back to the useStore, I used context because I thought it was more standard practice, but happy to use the existing pattern |
Either way works for me @gagdiez! I think the only important thing is to make sure we only have one source of truth for auth/wallet state and interactions (right now it's kind of split between |
@calebjacob migrated everything to use context. I might need some help with the |
@gagdiez Nice! This is how I've always interacted with it without const subscription = selector.store.observable.subscribe(async (value) => {
setState(value);
setAccount(value?.accounts[0] ?? null);
if (value.accounts.length > 0 && value.selectedWalletId && selector) {
const wallet = await selector.wallet();
setWallet(wallet);
} else {
setWallet(null);
}
}); Wherever you have cleanup or unmount logic, you can unsubscribe: subscription.unsubscribe(); |
@calebjacob thanks! I changed the code to not use |
Currently, the BOS VM is loaded and initialized on every single page - even those that do not depend on it. This creates a lot of delay for page loading, and even slows down the initialization of the wallet (which should be instant, and instead takes a couple of seconds waiting for the VM to load).
This PR separates the
wallet selector
(used to login) from the loading and initialization of the VM (used to render components), and makes sure that the VM is only loaded when a component has to be rendered.Separating the VM from the wallet selector allows a couple of improvements:
closes #1264