Newsletter: use with-site-settings.js #90605
Conversation
enejb
force-pushed
the
fix/newsletter-setting-saving-try-3
branch
from
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)
|
matticbot
added
[Status] Needs Review
enejb
force-pushed
the
fix/newsletter-setting-saving-try-3
branch
from
3ef99e9
to
63ba925
Compare
There was a problem hiding this comment.
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.
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.
There's some normalization of settings going on in the original Redux version:
Should this be carried here too?
| queryClient.invalidateQueries( { | ||
| queryKey: [ 'site-settings', siteId ], | ||
| } ); | ||
| queryOptions.onSuccess?.( ...args ); |
There was a problem hiding this comment.
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
| 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.
The existing wrapSettingsForm HoC has some complexity that we might not be considering here, like fetching Jetpack settings separately and combining them:
| const withSiteSettings = createHigherOrderComponent( ( Wrapped ) => { | ||
| const WithSiteSettings = ( props ) => { | ||
| const { siteId } = props; | ||
| const [ unsavedSettings, setUnsavedSettings ] = useState( {} ); |
There was a problem hiding this comment.
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.
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.
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.
Should we always use the same empty object instead of generating a new one?
| unsavedSettings | ||
| ); | ||
| return ( | ||
| <Wrapped |
There was a problem hiding this comment.
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.
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.
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 |
This PR is intended to fix #89109 issue.
It does so in a couple of ways.
with-site-settingsHOC that is intended to replace the currentwrap-settings-formthis new component used the useQuery component instead of the current redux.Proposed Changes
Testing Instructions
Pre-merge Checklist