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

Newsletter: use with-site-settings.js #90605

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
15 changes: 15 additions & 0 deletions client/data/site-settings/use-site-settings-query.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
import { useQuery } from '@tanstack/react-query';
import wp from 'calypso/lib/wp';

const useSiteSettingsQuery = ( siteId: number | null ) =>
Copy link
Member

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.

useQuery( {
queryKey: [ 'site-settings', siteId ],
queryFn: () => wp.req.get( `/sites/${ siteId }/settings`, { apiVersion: '1.4' } ),
Copy link
Member

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 };

refetchOnWindowFocus: false,
enabled: !! siteId,
meta: {
persist: false,
},
} );

export default useSiteSettingsQuery;
26 changes: 26 additions & 0 deletions client/data/site-settings/use-update-site-settings-mutation.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
import { useMutation, useQueryClient } from '@tanstack/react-query';
import { useCallback } from 'react';
import wp from 'calypso/lib/wp';

function useUpdateSiteSettingsMutation( siteId, queryOptions = {} ) {
const queryClient = useQueryClient();
const mutation = useMutation( {
mutationFn: ( { settings } ) =>
wp.req.post( '/sites/' + siteId + '/settings', { apiVersion: '1.4' }, settings ),
Copy link
Member

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?

...queryOptions,
onSuccess( ...args ) {
queryClient.invalidateQueries( {
queryKey: [ 'site-settings', siteId ],
} );
queryOptions.onSuccess?.( ...args );
Copy link
Member

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:

dispatch( requestSite( siteId ) );
dispatch( requestAdminMenu( siteId ) );

},
} );

const { mutate } = mutation;

const updateSiteSettings = useCallback( ( settings ) => mutate( { settings } ), [ mutate ] );

return { updateSiteSettings, ...mutation };
}

export default useUpdateSiteSettingsMutation;
151 changes: 151 additions & 0 deletions client/data/site-settings/with-site-settings.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,151 @@
import { useQueryClient } from '@tanstack/react-query';
import { createHigherOrderComponent } from '@wordpress/compose';
import { useCallback, useState } from '@wordpress/element';
import { useTranslate } from 'i18n-calypso';
import PropTypes from 'prop-types';
import { useDispatch, useSelector } from 'react-redux';
import useSiteSettingsQuery from 'calypso/data/site-settings/use-site-settings-query';
import useUpdateSiteSettingsMutation from 'calypso/data/site-settings/use-update-site-settings-mutation';
import { useProtectForm } from 'calypso/lib/protect-form';
import { recordGoogleEvent, recordTracksEvent } from 'calypso/state/analytics/actions';
import { errorNotice, successNotice } from 'calypso/state/notices/actions';
import getCurrentRouteParameterized from 'calypso/state/selectors/get-current-route-parameterized';

const noticeOptions = {
duration: 10000,
id: 'site-settings-save',
};

const withSiteSettings = createHigherOrderComponent( ( Wrapped ) => {
const WithSiteSettings = ( props ) => {
const { siteId } = props;
const [ unsavedSettings, setUnsavedSettings ] = useState( {} );
Copy link
Member

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:

Suggested change
const [ unsavedSettings, setUnsavedSettings ] = useState( {} );
const [ unsavedSettings, setUnsavedSettings ] = useState( () => {} );

const dispatch = useDispatch();
const translate = useTranslate();
const queryClient = useQueryClient();
const path = useSelector( ( state ) => getCurrentRouteParameterized( state, siteId ) );
const { markChanged, markSaved } = useProtectForm();

const trackEvent = useCallback(
( name ) => {
dispatch( recordGoogleEvent( 'Site Settings', name ) );
},
[ dispatch ]
);

const trackTracksEvent = useCallback(
Copy link
Member

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

( name, eventProps ) => {
dispatch( recordTracksEvent( name, eventProps ) );
},
[ dispatch ]
);

const { data, isLoading: isFetchingSettings } = useSiteSettingsQuery( siteId );

const updateSettings = useCallback(
( fields ) => {
markChanged();
return setUnsavedSettings( { ...unsavedSettings, ...fields } );
},
[ markChanged, unsavedSettings, setUnsavedSettings ]
);

const {
isPending: isSavingSettings,
updateSiteSettings: saveSettings,
context: mutateContext,
} = useUpdateSiteSettingsMutation( siteId, {
onMutate: async ( settings ) => {
const queryKey = [ 'site-settings', siteId ];
Copy link
Member

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.


// Cancel any current refetches, so they don't overwrite our optimistic update
await queryClient.cancelQueries( {
queryKey,
} );

// Snapshot the previous value
const previousSettings = queryClient.getQueryData( queryKey );

// Optimistically update to the new value
queryClient.setQueryData( queryKey, ( { settings: oldSettings } ) => {
return { ...oldSettings, ...settings };
} );

// Store previous settings in case of failure
return { previousSettings };
},
onSuccess() {
dispatch( successNotice( translate( 'Settings saved successfully!' ), noticeOptions ) );
setUnsavedSettings( {} ); // clear dirty fields after successful save.
Copy link
Member

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?

if ( path === '/settings/newsletter/:site' ) {
trackTracksEvent( 'calypso_settings_newsletter_saved', unsavedSettings );
}
markSaved();
},
onError( err, newSettings, context ) {
// Revert to previous settings on failure
queryClient.setQueryData( [ 'site-settings', siteId ], context.previousSettings );

dispatch(
errorNotice(
translate( 'There was a problem saving your changes. Please, try again.' ),
noticeOptions
)
);
},
onSettled: () => {
// Refetch settings regardless
queryClient.invalidateQueries( {
queryKey: [ 'site-settings', siteId ],
} );
},
} );

const handleSubmitForm = useCallback( () => {
trackEvent( 'Clicked Save Settings Button' );
saveSettings( unsavedSettings );
if ( ! unsavedSettings ) {
return;
}
trackEvent( 'Clicked Save Settings Button' );
}, [ saveSettings, unsavedSettings, trackEvent ] );

const handleToggle = useCallback(
( name ) => () => {
if ( unsavedSettings[ name ] === undefined ) {
unsavedSettings[ name ] = data?.settings[ name ];
}
updateSettings( { [ name ]: ! unsavedSettings[ name ] } );
trackEvent( `Toggled ${ name }` );
},
[ trackEvent, unsavedSettings, updateSettings, data?.settings ]
);

const settings = Object.assign(
{},
mutateContext?.previousSettings?.settings,
data?.settings,
unsavedSettings
);
return (
<Wrapped
Copy link
Member

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.

{ ...props }
settings={ settings }
isLoadingSettings={ isFetchingSettings }
isSavingSettings={ isSavingSettings }
updateSettings={ updateSettings }
unsavedSettings={ unsavedSettings }
handleToggle={ handleToggle }
handleSubmitForm={ handleSubmitForm }
/>
);
};

WithSiteSettings.propTypes = {
siteId: PropTypes.number.isRequired,
};

return WithSiteSettings;
}, 'WithSiteSettings' );

export default withSiteSettings;