-
Notifications
You must be signed in to change notification settings - Fork 9
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
Fix/fine-tune react-query settings #389
Conversation
Some thoughts:
If our primary concern is that the changes are overwritten when a user navigates away from the page, how about instead setting a
Makes sense. Should we create a separate file to import the queryClient object so that we don't have to instantiate a new object on every class? |
Updated description with conclusions from our meeting
Sure, we can put it in a context and call the context - how does that sound? |
b682342
to
339f23b
Compare
@gweiying upon further research, |
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.
lgtm!
97243c5
to
104585d
Compare
789315a
to
3edf305
Compare
473f913
to
104585d
Compare
583c455
to
71823a4
Compare
This commit sets the `refetchOnWindowFocus` flag for the useQuery on the following layouts/components to be `false`: - PageSettingsModal - EditNavBar - EditPage The reason we turn off this flag is because these pages involve user changes - with this flag on, any unsaved changes by the user would be overwritten by the refetched data. This commit also sets the - `refetchOnReconnect` - `refetchInterval` - `refetchIntervalInBackground` flags to be false for the same reasons explained above.
This commit adds cache invalidation of all our GET queries (the useQuery invocations) after we perform a mutation. This allows us to reset our cache in a more granular manner, as opposed to simply setting the cache time for our GET queries to be 0. Additionally, this commit also adds a PAGE_SETTINGS_KEY constant to replace the string literal that was being used as the query key for the PageSettingsModal. Refer to the following documentation for more details: - https://react-query.tanstack.com/guides/query-invalidation - https://react-query.tanstack.com/guides/invalidations-from-mutations
This commit standardizes the output of our GET API functions to return resp.data instead of just the response from the api call. resp.data is a better choice as we are able to use that in dependency arrays, whereas the resp object pointer always changes, which will trigger the useEffect every time even if the object data hasn't actually changed.
As per our discussion (refer to meeting minutes here: https://docs.google.com/document/d/1br6T6wVX0KrcA3nwQEo7OhUrcT4veLnaz0vByEXjVvo/edit#heading=h.hyx8t36v9z3n) we will be discussing our `useQuery` functions if there are changes to the local state that are being tracked, to avoid refetching behavior from overwriting local changes.
…or CollectionPagesSection
37cb353
to
61ea027
Compare
src/components/FolderModal.jsx
Outdated
onSuccess: () => { | ||
if (!isCollection) { | ||
// Resource folder | ||
console.log('hey') |
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.
to remove
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.
@@ -128,6 +131,10 @@ const Folders = ({ match, location }) => { | |||
setSelectedPage('') | |||
errorToast(`The page data could not be retrieved. ${DEFAULT_RETRY_MSG}`) | |||
}, | |||
onSuccess: () => { | |||
queryClient.invalidateQueries([DIR_CONTENT_KEY, siteName, folderName]) |
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.
to add successToast
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 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.
lgtm! thanks for making all the adjustments
* fix: turn off refetching on window focus for useQuery This commit sets the `refetchOnWindowFocus` flag for the useQuery on the following layouts/components to be `false`: - PageSettingsModal - EditNavBar - EditPage The reason we turn off this flag is because these pages involve user changes - with this flag on, any unsaved changes by the user would be overwritten by the refetched data. This commit also sets the - `refetchOnReconnect` - `refetchInterval` - `refetchIntervalInBackground` flags to be false for the same reasons explained above. * fix: invalidate queries after mutation This commit adds cache invalidation of all our GET queries (the useQuery invocations) after we perform a mutation. This allows us to reset our cache in a more granular manner, as opposed to simply setting the cache time for our GET queries to be 0. Additionally, this commit also adds a PAGE_SETTINGS_KEY constant to replace the string literal that was being used as the query key for the PageSettingsModal. Refer to the following documentation for more details: - https://react-query.tanstack.com/guides/query-invalidation - https://react-query.tanstack.com/guides/invalidations-from-mutations * feat: standardize output of GET API calls to return resp.data This commit standardizes the output of our GET API functions to return resp.data instead of just the response from the api call. resp.data is a better choice as we are able to use that in dependency arrays, whereas the resp object pointer always changes, which will trigger the useEffect every time even if the object data hasn't actually changed. * fix: load yaml content when reading directory file * feat: disable useQuery if component tracks local state As per our discussion (refer to meeting minutes here: https://docs.google.com/document/d/1br6T6wVX0KrcA3nwQEo7OhUrcT4veLnaz0vByEXjVvo/edit#heading=h.hyx8t36v9z3n) we will be discussing our `useQuery` functions if there are changes to the local state that are being tracked, to avoid refetching behavior from overwriting local changes. * fix: rebase errors * fix: invocation of LoginContext * Fix: rebase errors * Fix: update resource category and resource page get calls to return data directly * Refactor: use invalidateQueries instead of passing refetch function for CollectionPagesSection * Fix: error when retrieving page settings * Fix: rebase errors * Fix: invalidate query instead of reload when changing page settings * Feat: add success toast when changing settings * Fix: change folder naming to use invalidation instead of reload * Fix: invalidate correct resource folder key * Fix: update success toast messages * Fix: remove log statement * Fix: add successtoast Co-authored-by: Alexander Lee <alexander@open.gov.sg>
* Add linting and formatting tools (#378) * fix: outdated packages with vulnerabilities * feat: install eslint and initiate config * feat: install prettier and set prettier options * feat: install eslint-config-prettier * feat: install eslint-plugin-prettier * chore: reformat eslint config * feat: add @trivago/prettier-plugin-sort-imports, define preferred import order * fix: css-loader file resolution bug introduced by CRA v4 In recent commits, we upgraded our react-scripts version from 3.4.4 to 4.0.3. This is because CRA (create-react-app) v3 uses an outdated version of eslint (facebook/create-react-app#8849). This introduced a bug related to the css-loader library, which can no longer resolve assets in the public folder: - facebook/create-react-app#9870 (comment) - webpack-contrib/css-loader#1136 (comment) This commit fixes this bug by moving the referenced image to the relevant sub-directory in the src directory. * chore: temporarily disable eslint * chore: add more files and folders to .prettierignore * chore: upgrade prettier-plugin-sort-imports to 2.0.2 fixes trivago/prettier-plugin-sort-imports#22 * chore: temporarily disable prettier * chore: remove prettier config temporarily * chore: remove jsx-a11y references temporarily * temporarily remove import/prefer-default-export reference Co-authored-by: jiehao <jiehao@open.gov.sg> Co-authored-by: Preston Lim <prestonlimlianjie@gmail.com> * Fix/resource color (#430) * fix: resource page header changed to bg-secondary * fix: using isResourcePage to determine page header * Fix/rearrange layout (#427) * fix: simplify directoryFile utils for retrieve and update * fix: update methods using directoryFile utils * fix: introduce FolderReorderingModal * fix: refactors FolderContent * fix: update params for FolderContentItem * fix: fix breadcrumb display * fix: add propTypes for FolderReorderingModal * fix: add Cancel button to FolderReorderingModal * fix: updates draggable-id for React dnd * fix: updates dropdown button behavior for reordering * fix: updates copy text * fix: updates copy text * fix: updates variable naming for directory file output * refactor: clean up ProtectedRoute and LoginContext (#431) * refactor: clean up ProtectedRoute and LoginContext * refactor: make LoginContext testable create 3 exports: LoginContext itself, LoginProvider, LoginConsumer * refactor: make route selector testable in App.jsx * refactor: group routing components * feat: add basic routing tests * refactor: move __tests__ folder to correct place Required for jest to find the test files * style: delete unnecessary div * refactor: make exports more obvious for LoginContext.js * refactor: delete duplicate route * chore: add rest of routing tests * style: remove unused declarations * Fix/fine-tune react-query settings (#389) * fix: turn off refetching on window focus for useQuery This commit sets the `refetchOnWindowFocus` flag for the useQuery on the following layouts/components to be `false`: - PageSettingsModal - EditNavBar - EditPage The reason we turn off this flag is because these pages involve user changes - with this flag on, any unsaved changes by the user would be overwritten by the refetched data. This commit also sets the - `refetchOnReconnect` - `refetchInterval` - `refetchIntervalInBackground` flags to be false for the same reasons explained above. * fix: invalidate queries after mutation This commit adds cache invalidation of all our GET queries (the useQuery invocations) after we perform a mutation. This allows us to reset our cache in a more granular manner, as opposed to simply setting the cache time for our GET queries to be 0. Additionally, this commit also adds a PAGE_SETTINGS_KEY constant to replace the string literal that was being used as the query key for the PageSettingsModal. Refer to the following documentation for more details: - https://react-query.tanstack.com/guides/query-invalidation - https://react-query.tanstack.com/guides/invalidations-from-mutations * feat: standardize output of GET API calls to return resp.data This commit standardizes the output of our GET API functions to return resp.data instead of just the response from the api call. resp.data is a better choice as we are able to use that in dependency arrays, whereas the resp object pointer always changes, which will trigger the useEffect every time even if the object data hasn't actually changed. * fix: load yaml content when reading directory file * feat: disable useQuery if component tracks local state As per our discussion (refer to meeting minutes here: https://docs.google.com/document/d/1br6T6wVX0KrcA3nwQEo7OhUrcT4veLnaz0vByEXjVvo/edit#heading=h.hyx8t36v9z3n) we will be discussing our `useQuery` functions if there are changes to the local state that are being tracked, to avoid refetching behavior from overwriting local changes. * fix: rebase errors * fix: invocation of LoginContext * Fix: rebase errors * Fix: update resource category and resource page get calls to return data directly * Refactor: use invalidateQueries instead of passing refetch function for CollectionPagesSection * Fix: error when retrieving page settings * Fix: rebase errors * Fix: invalidate query instead of reload when changing page settings * Feat: add success toast when changing settings * Fix: change folder naming to use invalidation instead of reload * Fix: invalidate correct resource folder key * Fix: update success toast messages * Fix: remove log statement * Fix: add successtoast Co-authored-by: Alexander Lee <alexander@open.gov.sg> * fix: pass parameters to wrapped components (#439) * fix: fixes toast popup on item select, folder deletion modal (#440) * fix: fixes toast popup on item select, folder deletion modal * fix: add condition for folder deletion Co-authored-by: kwajiehao <31984694+kwajiehao@users.noreply.github.com> Co-authored-by: jiehao <jiehao@open.gov.sg> Co-authored-by: Preston Lim <prestonlimlianjie@gmail.com> Co-authored-by: Alexander Lee <alexander@open.gov.sg> Co-authored-by: Alexander Lee <alexleegs@gmail.com>
This PR was reviewed with the
staging
branch of the backend.Since changes are being made to the
PageSettingsModal
in PR #388, this PR should be rebased ontostaging
after #388 is merged intostaging
before being reviewed.Overview
This PR makes 2 major updates to how we use the
react-query
library in our codebase:Turning off automatic refetching behavior fordisableuseQuery
if changes have been made to local stateuseQuery
after a mutation1.
Turning off automatic refetching forDisableuseQuery
if changes to local state are madeSome of the components which we use
useQuery
in require us to maintain content state to track changes to the content that users have made (for example, inPageSettingsModal
, we need to maintain local state to track changes to the page title that the users make).However, currently, any changes made by the user are reset if the user navigates to another tab/window, and then returns to the CMS. This is reproducible through the following steps:
EditPage
layoutThis is because
react-query
refetched the page content and overwrote the changes to local state.Therefore, it's important to turn off refetching so we don't lose changes to local state made by the user.We discussed this problem in depth, and the discussion minutes can be found in the CMS working document here (link). The conclusion was to keep a state variable,
hasChanges
, for detecting whether there are any changes to the local state from the initial mounted state. If there are changes, thenhasChanges
should be set totrue
, and we disableuseQuery
.This solution is preferable because in most situations where we maintain local state and track local changes, we already check for changes in state to determine whether we should show a warning modal for users who attempt to navigate away with unsaved changes.
2. Performing query cache invalidation for corresponding
useQuery
after a mutationSince the CMS needs to display the latest information to users, we need to ensure that our
useQuery
functions are not fetching outdated, cached data. Therefore, when we perform any mutations, we should invalidate the cache for the corresponding GET query so that we fetch fresh data the next time.The alternative to this would be to set
cacheTime
to be 0 for alluseQuery
functions. Manual cache invalidation is more fine-grained and allows us to benefit from some degree of caching. Personally I'm open to either approach.Misc. bugs fixed in this PR
resp.data
instead of just the response from the api call.resp.data
is a better choice as we are able to use it inuseEffect
dependency arrays, whereas theresp
object always changes, which will trigger theuseEffect
every time even if the object data hasn't actually changed.