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

Implement autostart for VNet in Connect #40900

Merged
merged 9 commits into from
Apr 26, 2024
Merged

Implement autostart for VNet in Connect #40900

merged 9 commits into from
Apr 26, 2024

Conversation

ravicious
Copy link
Member

@ravicious ravicious commented Apr 25, 2024

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 from StatePersistenceState 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.

const [{ autoStart }, setAppState] = usePersistedState('vnet', {
  autoStart: false,
});

Unlike useState, usePersistedState doesn't let you define arbitrary state and still requires you to add any new fields to StatePersistanceState. That gives us more control over what exactly gets stored in app_state.json and it help with avoiding typos (e.g. I cannot do usePersistedState('vnet') in one place and usePersistedState('VNet') in another).

One big caveat is that at the moment it doesn't propagate updates across multiple usePersistedState callsites, see the comment above usePersistedState 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 and WorkspacesService states to be initialized.

To achieve that, I added isInitialized to the state of WorkspacesService. But how does a React component get "notified" about isInitialized changing from false to true?

Historically, we used useState method of each service which underneath called useStore. This hook updates the component whenever the state of a service changes. For isInitialized 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's useSyncExternalStore together with subscribeWithSelector that I added to ImmutableStore 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:

const isWorkspaceStateInitialized = useStoreSelector(
  'workspacesService',
  useCallback(state => state.isInitialized, [])
);

In the future, it can be used to optimize some of our other hooks. For example, useLoggedInUser and useWorkspaceLoggedInUser today call useState on clustersService and workspacesService. With useStoreSelector, 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 and useState over custom solutions.

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.
@ravicious ravicious added the no-changelog Indicates that a PR does not require a changelog entry label Apr 25, 2024
@github-actions github-actions bot requested a review from ryanclark April 25, 2024 09:59
@ravicious ravicious removed the request for review from ryanclark April 25, 2024 09:59
Copy link
Contributor

@gzdunek gzdunek left a 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<
Copy link
Contributor

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?).

Copy link
Member Author

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.

Copy link
Member Author

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.

@ravicious ravicious added this pull request to the merge queue Apr 26, 2024
Merged via the queue into master with commit e12c54b Apr 26, 2024
40 checks passed
@ravicious ravicious deleted the r7s/vnet-autostart branch April 26, 2024 09:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-changelog Indicates that a PR does not require a changelog entry size/md ui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants