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
Newsletter: use with-site-settings.js #90605
Conversation
9349408
to
469e27d
Compare
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: Sections (~809 bytes added 📈 [gzipped])
Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to. Legend What is parsed and gzip size?Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory. Generated by performance advisor bot at iscalypsofastyet.com. |
Jetpack Cloud live (direct link)
Automattic for Agencies live (direct link)
|
3ef99e9
to
63ba925
Compare
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.
Nice work overall, thanks @enejb!
However, do you have a migration plan from the Redux version of site settings for the rest of the settings pages?
RIght now, maintaining the site settings data in both React Query and Redux, and that can lead to subtle data sync issues. That can be fine if it's temporary, while migrating away the rest of the settings pages. However, if we're not planning to refactor the rest of the settings pages, we might be stuck with duplicated settings data forever, which I'd strongly recommend against.
import { useQuery } from '@tanstack/react-query'; | ||
import wp from 'calypso/lib/wp'; | ||
|
||
const useSiteSettingsQuery = ( siteId: number | 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.
In general, it's nice to see this refactored to React Query 👍
I'm a bit concerned about the duplication though, as all settings pages could take a long time to be refactored.
const queryClient = useQueryClient(); | ||
const mutation = useMutation( { | ||
mutationFn: ( { settings } ) => | ||
wp.req.post( '/sites/' + siteId + '/settings', { apiVersion: '1.4' }, settings ), |
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.
There's some normalization of settings going on in the original Redux version:
dispatch( updateSiteSettings( siteId, normalizeSettings( body.updated ) ) ); |
Should this be carried here too?
queryClient.invalidateQueries( { | ||
queryKey: [ 'site-settings', siteId ], | ||
} ); | ||
queryOptions.onSuccess?.( ...args ); |
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.
We might need to invalidate a few more things on success:
There's some normalization of settings going on in the original Redux version:
wp-calypso/client/state/site-settings/actions.js
Lines 100 to 101 in e1f240e
dispatch( requestSite( siteId ) ); | |
dispatch( requestAdminMenu( siteId ) ); |
const useSiteSettingsQuery = ( siteId: number | null ) => | ||
useQuery( { | ||
queryKey: [ 'site-settings', siteId ], | ||
queryFn: () => wp.req.get( `/sites/${ siteId }/settings`, { apiVersion: '1.4' } ), |
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.
The existing wrapSettingsForm
HoC has some complexity that we might not be considering here, like fetching Jetpack settings separately and combining them:
settings = { ...settings, ...jetpackSettings }; |
const withSiteSettings = createHigherOrderComponent( ( Wrapped ) => { | ||
const WithSiteSettings = ( props ) => { | ||
const { siteId } = props; | ||
const [ unsavedSettings, setUnsavedSettings ] = useState( {} ); |
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.
Nit: this creates a new object on every rerender. You might want to use lazy init:
const [ unsavedSettings, setUnsavedSettings ] = useState( {} ); | |
const [ unsavedSettings, setUnsavedSettings ] = useState( () => {} ); |
[ dispatch ] | ||
); | ||
|
||
const trackTracksEvent = useCallback( |
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.
Not sure these wrappers add much value. I'd likely just use dispatch( recordTracksEvent(...) )
.
context: mutateContext, | ||
} = useUpdateSiteSettingsMutation( siteId, { | ||
onMutate: async ( settings ) => { | ||
const queryKey = [ 'site-settings', siteId ]; |
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.
Query key could be abstracted to a reusable function to ensure we're using the same one.
}, | ||
onSuccess() { | ||
dispatch( successNotice( translate( 'Settings saved successfully!' ), noticeOptions ) ); | ||
setUnsavedSettings( {} ); // clear dirty fields after successful save. |
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.
Should we always use the same empty object instead of generating a new one?
unsavedSettings | ||
); | ||
return ( | ||
<Wrapped |
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.
If we're doing this "the new way", it might be more convenient to just use a hook instead of creating a HoC.
|
||
const NewsletterSettings = () => { | ||
const translate = useTranslate(); | ||
|
||
const siteId = useSelector( getSelectedSiteId ) ?? 0; |
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.
Why is it necessary to fall back to 0
? Couldn't this bleed somewhere unexpected and cause issues?
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.
It is not I was just running into issues with typescript when I used const siteId = useSelector( getSelectedSiteId )
Can you also elaborate on what the fix is? Can we do the fix in a PR, and then do a migration to React Query in another one? |
Thanks for the review @tyxla.
I am not able to reproduce this issue any more and I am not sure how it was fixed. :( When I was trying to fix this issue I notice a couple of things.
I tried to fix this issue by addressing a creating a bunch of Prs. (#90531, #90294 and Automattic/jetpack#37190) But in the end I try to simply the react code using this PR that would make it easier to reason about and reduce the unintended consequences.
My plan is to create a drop in replacement for I do agree that having state in two different places is not great. So I wanted to get your input on this :) I think we could proceed in two different ways.
I would love to hear your thoughts on this :) I am happy to address the feedback after we come up with a plan |
If you have time to refactor all components that use |
I am going to close this for now. I do think |
This PR is intended to fix #89109 issue.
It does so in a couple of ways.
with-site-settings
HOC that is intended to replace the currentwrap-settings-form
this new component used the useQuery component instead of the current redux.Proposed Changes
Testing Instructions
Pre-merge Checklist