-
-
Notifications
You must be signed in to change notification settings - Fork 5.4k
Add support for offline/local first applications #10545
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
base: next
Are you sure you want to change the base?
Conversation
EditPostPage.setInputValue( | ||
'input', | ||
'title', | ||
'Lorem Ipsum again{enter}' |
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.
Because react-query now persist queries and mutations for offline mode, the previous test now leaks into the second (e.g. this post has its title changed to Lorem Ipsum). I tried to configure testIsolation in Cypress but our version is probably too old
d440e8e
to
9d66e05
Compare
I can't see a way to test this except e2e. Not sure if we can simulate network loss with cypress though |
@djhi since the simple demo has the react-query DevTools enabled, you can use them to simulate network loss I believe (clicking on the 'Wifi' icon does the job). A bit ugly but it should work! |
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.
Praise: very understandable doc !!
packages/ra-core/src/controller/input/useReferenceInputController.ts
Outdated
Show resolved
Hide resolved
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.
Only reviewed the docs and the simple demo so far...
docs/DataProviders.md
Outdated
|
||
export const queryClient = new QueryClient(); | ||
|
||
const queryClientWithOfflineSupport = addOfflineSupportToQueryClient({ |
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.
What's the point of queryClientWithOfflineSupport
variable? Why is it not exported instead of queryClient
? Is queryClient
mutated when you use addOfflineSupportToQueryClient
, so exporting it is not required? Current variable naming makes me think that queryClient
doesn't have offline support.
EDIT: in following examples, it's much clearer.
const queryClientWithOfflineSupport = addOfflineSupportToQueryClient({ | |
const baseQueryClient = new QueryClient(); | |
export const queryClient = addOfflineSupportToQueryClient({ | |
queryClient, | |
dataProvider, | |
resources: ['posts', 'comments'], | |
}); |
const translate = useTranslate(); | ||
|
||
if (isPaused && (data == null || isPlaceholderData)) return <Offline />; |
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.
Maybe add a quick example about this in docs? At least something mentioning that you can use isPaused
and isPlaceholderData
to detect that you're in offline mode.
@@ -61,6 +61,7 @@ export const useReferenceArrayInputController = < | |||
error: errorGetMany, | |||
isLoading: isLoadingGetMany, | |||
isFetching: isFetchingGetMany, | |||
isPaused: isPausedGetMany, | |||
isPending: isPendingGetMany, | |||
refetch: refetchGetMany, | |||
} = useGetManyAggregate<RecordType>( |
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.
No isPlaceholderData
here? What's the difference with useReferenceArrayFieldController
?
* </PersistQueryClientProvider> | ||
* ); | ||
* | ||
* @example <caption>Adding offline support with custom mutations</caption> |
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.
Aren't these examples mixed up? The first one shows with a custom mutation and the second one without it.
} | ||
|
||
interface ComponentsPropsList { | ||
RaOffline: Partial<Offline>; |
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.
Use [PREFIX]
instead of 'RaOffline'
.
|
||
interface Components { | ||
RaOffline?: { | ||
defaultProps?: ComponentsPropsList['RaOffline']; |
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.
Use typeof PREFIX
instead of 'RaOffline'
.
</Edit> | ||
); | ||
|
||
export const Offline = ({ dataProvider = dataProviderWithAuthors }) => ( |
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.
Tested it and the default AutocompleteInput
shows "Loading..." instead of the offline component that I expected. I think you need to change useChoicesContext
to propagate isPaused
and isPlaceholderData
to show a better message in offline mode.
title={ | ||
<Translate | ||
i18nKey="ra.notifications.pending_operations" | ||
options={{ smart_count: 1 }} |
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 { smart_count: 1 }
? Shouldn't it be { smart_count: pendingMutations.length }
?
); | ||
|
||
export const Offline = () => ( | ||
<Wrapper listContext={{ isPaused: true }}> |
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.
This story shows nothing. Is it supposed to show the offline component?
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.
Reviewing the whole PR was hard because it's very big and impacts many components. Likewise, I guess it's hard to keep track of the review comments.
Thant's why I tried to mark as Resolved all comments that I believe are already addressed, but there are still some here and there that still need discussion. So I recommend you go through all of them once more to make sure you're not missing something important.
I also share the comments about inconsistencies regarding isPlaceholderData
, for instance I don't understand why useReferenceOneFieldController
uses it but not useEditController
? Can you clarify your rationale a bit?
That being said, according to my tests, all features work as expected! So well done!! 💪
yarn.lock
Outdated
"@tanstack/query-core@npm:5.81.2": | ||
version: 5.81.2 | ||
resolution: "@tanstack/query-core@npm:5.81.2" | ||
checksum: 36a6bddec2e7512015bcfbb0d7b0876fab418de9e0ef21ad403598276960e0b7d53efd62832ce462738ad22d9883e31cb5403eafc65dfd9b2f6744c22a9d8e42 | ||
languageName: node | ||
linkType: hard |
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.
needs dedupe
docs/DataProviders.md
Outdated
|
||
## Handling Errors For Resumed Mutations | ||
|
||
If you enabled offline support, users might trigger mutations while being actually offline. When they're back online, react-query will _resume_ those mutations and they might fail for other reasons (server side validation or errors). However, as users might have navigated away from the page that triggered the mutation, they won't see any notification. |
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 you enabled offline support, users might trigger mutations while being actually offline. When they're back online, react-query will _resume_ those mutations and they might fail for other reasons (server side validation or errors). However, as users might have navigated away from the page that triggered the mutation, they won't see any notification. | |
If you enabled offline support, users might trigger mutations while being actually offline. When they're back online, TanStack Query will _resume_ those mutations and they might fail for other reasons (server side validation or errors). However, as users might have navigated away from the page that triggered the mutation, they won't see any notification. |
React.useEffect(() => { | ||
const handleChange = () => { | ||
setIsOnline(onlineManager.isOnline()); | ||
}; | ||
return onlineManager.subscribe(handleChange); | ||
}, []); |
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.
This code looks odd... Shouldn't you subscribe on mount and unsubscribe on unmount? 🤔
const notify = useNotify(); | ||
|
||
React.useEffect(() => { | ||
const mutationKeyFilter = []; // An empty array targets all mutations |
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.
mutationKeyFilter
is used nowhere I believe 🤔
useEffect(() => { | ||
mode.current = mutationMode; | ||
}, [mutationMode]); |
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 do you need that? 🤔
Why only in useCreate? 🤔
if (!resource) { | ||
throw new Error( | ||
'<Offline> must be used inside a <Resource> component or provided a resource prop' | ||
); | ||
} |
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.
This limitation seems odd. If it is only required for the translation string, I think there are easy solutions to remove this limitation.
label={props.label} | ||
resource={props.resource} | ||
> | ||
{offline ?? <Offline variant="inline" />} |
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 inline? 🤔
if (controllerProps.isPaused && controllerProps.referenceRecord == null) { | ||
return offline; | ||
} | ||
|
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 would be nice to have a story to test this component too 😇
Problem
React-admin currently don't handle well the loss of network connectivity: in any demo, if you go to a list page, then simulate being offline using the devtool and try to go to the second page of the same list, it will still display the first page data while the pagination will indeed indicate you are on page 2.
We handled it a bit better by at least showing users a notification when they try to load new data while offline but we can do better and allow for resumable mutations.
Solution
ListContext
include theisPaused
andisPlaceholderData
props from react-query and update components/hooks accordinglyempty
component when those two props are true (ideally we should have a dedicated component for that or handle the case in the default empty but this is achievable in userland)mutationKey
to all mutations so that we can provide default functions for them at thequeryClient
level (required by react-query: https://tanstack.com/query/latest/docs/framework/react/guides/mutations#persisting-offline-mutations)offline
prop to reference fields componentsoffline
prop to list componentsoffline
prop to show and edit viewsoffline
prop to reference inputs componentsScreenshots
Edit view

Show view

DataTable (same for all list child components

How To Test
Then:
optimistic
option in the form toolbar first.undoable
option in the form toolbar first.f00bar
as the post title to trigger an error once the network is online.In stories for list components, reference components and detail views, simply switch to
offline
and hit the storybook refresh buttonAdditional Checks
master
for a bugfix, ornext
for a featureAlso, please make sure to read the contributing guidelines.