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

Add support for mutationMode in useCreate #10530

Open
wants to merge 6 commits into
base: next
Choose a base branch
from
Open

Conversation

djhi
Copy link
Collaborator

@djhi djhi commented Feb 19, 2025

Problem

We currently don't support optimistic and undoable mutationMode in useCreate, which prevent us from supporting offline first applications.

Solution

Given that the new record identifier is generated client side, we can support optimistic and undoable mutationMode.

How To Test

Additional Checks

  • The PR targets master for a bugfix, or next for a feature
  • The PR includes unit tests (if not possible, describe why)
  • The PR includes one or several stories (if not possible, describe why)
  • The documentation is up to date

Also, please make sure to read the contributing guidelines.

@erwanMarmelab
Copy link
Contributor

Thank you for the full / nice doc and tests

@guilbill guilbill self-requested a review February 24, 2025 08:56
@guilbill guilbill changed the base branch from master to next February 24, 2025 08:57
mode.current = mutationMode;
}

if (returnPromise && mode.current !== 'pessimistic') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it needed ? as the returnPromise is defined base on the mode

returnPromise: mutationMode === 'pessimistic',

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the controller yes, but this is useCreate which can be used independently

@@ -50,6 +50,7 @@ const AccessControlAdmin = ({ queryClient }: { queryClient: QueryClient }) => {
const [resourcesAccesses, setResourcesAccesses] = React.useState({
'books.list': true,
'books.create': false,
'books.delete': false,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't get why we need this, is it a fix ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a fix to avoid error messages in the tests output. Just a cleanup really

docs/Create.md Outdated

- `pessimistic`: The mutation is passed to the dataProvider first. When the dataProvider returns successfully, the mutation is applied locally, and the side effects are executed.
- `optimistic`: The mutation is applied locally and the side effects are executed immediately. Then the mutation is passed to the dataProvider. If the dataProvider returns successfully, nothing happens (as the mutation was already applied locally). If the dataProvider returns in error, the page is refreshed and an error notification is shown.
- `undoable` (default): The mutation is applied locally and the side effects are executed immediately. Then a notification is shown with an undo button. If the user clicks on undo, the mutation is never sent to the dataProvider, and the page is refreshed. Otherwise, after a 5 seconds delay, the mutation is passed to the dataProvider. If the dataProvider returns successfully, nothing happens (as the mutation was already applied locally). If the dataProvider returns in error, the page is refreshed and an error notification is shown.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default one is 'pessimistic' if I'm right


```jsx
// In optimistic mode, ids must be generated client side
const id = uuid.v4();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was wondering if it was common to do it like this, generating the id client side. I understand the why, but I'm not sure on the how we solve this. Couldn't we have a mechanism witch a temporary id, replacing it as the data is fetched ?

Copy link
Collaborator Author

@djhi djhi Feb 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it is common in offline/local first scenarios. A temporary mechanism would make handling relationships such as those with ReferenceOneInput or ReferenceManyToManyInput a nightmare. This is a good starting point and if we do find a way to use temporary id, we can add it later.

Copy link
Contributor

@guilbill guilbill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests and documentation are very clean! thanks for that, although, some errors in the doc, and I have some question about the optimistic mode and the fact that we generate the id client side.

```tsx
const notify = useNotify();
const redirect = useRedirect();
const like = { postId: record.id };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nipitck: the like should have an id if you want to use mutationMode: 'optimistic'

Comment on lines +308 to +313
const [create] = useCreate(
'posts',
{ id: record.id, data: { isPublished: true } },
{ returnPromise: true }
);
const [create] = useCreate('auditLogs');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can't declare two variables called create 🤣

@@ -115,6 +115,7 @@ export const Admin = (props: AdminProps) => {
loginPage,
notification,
queryClient,
QueryClientProvider,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't we document this new prop?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was commited by mistake

Comment on lines +272 to +274
act(() => {
screen.getByText('Create post').click();
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: why do you need act? 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

copy/pasted from useUpdate

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I confirm it's unnecessary

Comment on lines +173 to +181
res && res.pages
? {
...res,
pages: res.pages.map(page => ({
...page,
data: updateColl(page.data),
})),
}
: res,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you adding the new record to all pages of the infinite query? Won't that lead to the record appearing multiple times in the list? 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we can probably be smarter here and only add it at the top. Same for the list actually where we should only add it to the current page

Comment on lines +160 to +165
queryClient.setQueriesData(
{ queryKey: [resource, 'getList'] },
(res: GetListResult) =>
res && res.data ? { ...res, data: updateColl(res.data) } : res,
{ updatedAt }
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't the total be updated to total + 1?

Comment on lines +190 to +197
queryClient.setQueriesData(
{ queryKey: [resource, 'getManyReference'] },
(res: GetListResult) =>
res && res.data
? { data: updateColl(res.data), total: res.total }
: res,
{ updatedAt }
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't the total be updated to total + 1?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RFR Ready For Review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants