-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Implement autostart for VNet in Connect #40900
Conversation
Those tests explicitly set the entire WorkspacesService state and would have to be modified each time we added something to the state. There's no reason for them to know about WorkspacesService state, so this commit refactors them out of that knowledge.
The unsubscribe function will be needed in order to comply with useSyncExternalStore API in the next commit.
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.
Awesome, these new hooks make working with non-react services much nicer 👏
I only left a comment about naming.
* ); | ||
* } | ||
*/ | ||
export function useAppState< |
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.
I have a feeling that this name could be more specific, for example, usePersistedState
or useAppPersistedState
.
Without checking the docs it is quite hard to guess what kind of state it updates (maybe it's a runtime state that lives in AppContext
services?).
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.
Renamed to usePersistedState
after a short discussion in DMs.
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.
…and renamed useImmutableStore
to useStoreSelector
, because as Grzegorz pointed out to me, the "immutable" part is just an implementation details.
Colleagues, comrades, fellow developers, engineering directors, Mr. president. I have a very exciting PR to share with you today.
On the surface, it merely implements autostart for VNet, which is similar to how autostart works in Connect My Computer: it's turned on when VNet starts, turned off when VNet is manually stopped. The exciting part is two new hooks introduced in this PR. I believe those hooks will make future work on Connect easier and more idiomatic in the context of React.
usePersistedState
The state of autostart needs to be stored on disk. Historically this has been done by adding a new field to
StatePersistenceState
, and then adding both a getter and a setter. That's a lot of boilerplate and worst of all, it doesn't quite follow React's state model. Most of the time your component gets a fresh value fromStatePersistenceState
only because some other part of the component has caused an update, but changing the state on disk doesn't update anything.usePersistedState
is supposed to be a first attempt at curbing that.Unlike
useState
,usePersistedState
doesn't let you define arbitrary state and still requires you to add any new fields toStatePersistanceState
. That gives us more control over what exactly gets stored inapp_state.json
and it help with avoiding typos (e.g. I cannot dousePersistedState('vnet')
in one place andusePersistedState('VNet')
in another).One big caveat is that at the moment it doesn't propagate updates across multiple
usePersistedState
callsites, see the comment aboveusePersistedState
definition. At the moment it's not a problem since the app state is consumed at a single callsite anyway (VnetContext
).useStoreSelector
Accessing resources through VNet might require Connect to show some modals and some other UI interactions (i.e. showing recent connections in the future). As such, we cannot autostart VNet the moment the context gets mounted. We have to wait for
ClustersService
andWorkspacesService
states to be initialized.To achieve that, I added
isInitialized
to the state ofWorkspacesService
. But how does a React component get "notified" aboutisInitialized
changing fromfalse
totrue
?Historically, we used
useState
method of each service which underneath calleduseStore
. This hook updates the component whenever the state of a service changes. ForisInitialized
in particular, this would be very wasteful –isInitialized
changes only once, but the rest of the workspace state changes all the time when you open new tabs.useStoreSelector
fixes that. It uses React'suseSyncExternalStore
together withsubscribeWithSelector
that I added toImmutableStore
a while ago. This means you can write the following code and it'll cause the component to update only when the selected part of the state gets updated:In the future, it can be used to optimize some of our other hooks. For example,
useLoggedInUser
anduseWorkspaceLoggedInUser
today calluseState
onclustersService
andworkspacesService
. WithuseStoreSelector
, they could provide selectors that trigger updates only when the relevant part of the state changes.For services which need to be consumed outside of React code, it's probably a good idea to keep their
ImmutableStore
implementations and don't migrate completely to React. But for everything else, we should probably use React built-ins such as contexts anduseState
over custom solutions.