Remove user-data-updated listener in useSiteDetails#2164
Remove user-data-updated listener in useSiteDetails#2164fredrikekelund merged 2 commits intotrunkfrom
user-data-updated listener in useSiteDetails#2164Conversation
| const selectedSite = useMemo( () => { | ||
| const site = data.find( ( site ) => site.id === selectedSiteId ) || firstSite; | ||
| if ( addingSiteIds.includes( site?.id ) ) { | ||
| site.isAddingSite = true; |
There was a problem hiding this comment.
This is where we previously mutated the site object to add the isAddingSite property, but we didn't have any explicit logic to unset it.
| let updatedSite = { ...newSite }; | ||
|
|
||
| if ( newSite.phpVersion !== phpVersion ) { | ||
| updatedSite = { | ||
| ...updatedSite, | ||
| phpVersion, | ||
| }; | ||
| } | ||
|
|
||
| if ( updatedSite !== newSite ) { | ||
| await updateSite( updatedSite ); | ||
| } |
There was a problem hiding this comment.
This is where we previously set the PHP version and updated the site list state in useSiteDetails by calling updateSite.
I removed this logic and opted for passing the PHP version as an argument to createSite instead.
| setSites( ( prevData ) => | ||
| prevData.map( ( site ) => { | ||
| if ( site.id === newSite.id ) { | ||
| return { ...newSite, isAddingSite: true }; | ||
| } | ||
| return site; | ||
| } ) |
There was a problem hiding this comment.
This call—and the equivalent one just below this in the diff—is what replaces the addingSiteIds state.
| const selectedSite = useMemo( () => { | ||
| const site = data.find( ( site ) => site.id === selectedSiteId ) || firstSite; | ||
| if ( addingSiteIds.includes( site?.id ) ) { | ||
| site.isAddingSite = true; | ||
| } | ||
| return site; | ||
| }, [ addingSiteIds, data, firstSite, selectedSiteId ] ); |
There was a problem hiding this comment.
After removing the site.isAddingSite = true assignment, there was no longer any point in using useMemo here (see this classic blog post)
📊 Performance Test ResultsComparing 55e7d67 vs trunk site-editor
site-startup
Results are median values from multiple test runs. Legend: 🟢 Improvement (faster) | 🔴 Regression (slower) | ⚪ No change |
sejas
left a comment
sejas
left a comment
There was a problem hiding this comment.
Thanks for creating the proper solution. I confirm the Add site creation works as expected, I also tested editing a the site name and PHP version. Also, thanks for the bonuses. The code is much cleaner.
fredrikekelund
deleted the
f26d/remove-user-data-listener-use-site-details
branch
|
Nice cleanup! 👍 |
3 participants
Related issues
studio preview createto adjust new site logic #2148)Proposed Changes
The data flow in and around
useSiteDetailswhen creating a new site is pretty gnarly. Previously, we were unintentionally relying on theuser-data-updatedlistener to refresh the state after writing to the appdata file. When I removed this listener in #2148, it caused a regression in which theisAddingSiteproperty on the new site never cleared (which meant the loading screen would display indefinitely). @sejas merged a hotfix for this in #2161.The
user-data-updatedlistener is meant to let Studio pick up changes from the CLI. We should not rely on it for flows that run entirely in Studio, where we already have all the state easily accessible in memory.This PR removes the
user-data-updatedlistener again and fixes the underlying problem by:selectedSiteobject in theuseMemocallback (this is where we previously set theisAddingSiteproperty, but we didn't have any explicit logic to unset it)createSitefunction inuseSiteDetailsto accept aphpVersionargument. Previously, we would set this in a callback function inuseAddSitethat included a state update inuseSiteDetails(which would unsetisAddingSiteflag).addingSiteIdsstate and instead explicitly setting and unsetting theisAddingSiteflag on the site object in thecreateSitefunction.useMemoto storeselectedSite. The way we did this had no effect.datatositesinuseSiteDetails.Testing Instructions
Pre-merge Checklist