Unexpected conversion of Date to string in optimistic updates in useUpdate and Forms

[kafendt]

What you were expecting:
Hey there, first off, thank you very much for all the great work you are doing on react-admin. I very much appreciate it and love what you have built!

I am experiencing problems due to how react-admin handles optimistic updates and just wanted to raise awarness for this.
The problem lies in the code below here:

react-admin/packages/ra-core/src/dataProvider/useUpdate.ts

Line 127 in 107db17

const clonedData = JSON.parse(JSON.stringify(data));

        // Stringify and parse the data to remove undefined values.
        // If we don't do this, an update with { id: undefined } as payload
        // would remove the id from the record, which no real data provider does.
        const clonedData = JSON.parse(JSON.stringify(data));

I have objects in the form of:

interface Appointment {
  start: Date
  end: Date
}

Now, I have a form which i use to update these appointments which also outputs start and end as javascript Date types.
I was expecting, that optimisic updates based on this would keep the types intact and save an updated object of the same shape into the query cache.

What happened instead:

When react-admin performs its optimisic update and updates the @tanstack/react-query cache, it does it using the code snippet above to strip all undefined values. Unfortunately in the process, through the JSON.stringify, it also transforms my Dates into strings.
So now I have an appointment in the form of:

interface Appointment {
  start: string
  end: string
}

in my cache until the response from the server comes along and places a new object with Dates in it again. Unfortunately in the meantime the date from the optimistic updates, leads to unexpected behaviour if it isn't handled correctly.

My question now is: If the only purpose of the JSON.parse(JSON.stringify(...)) is, to remove undefined values. Wouldn't it make sense to recursively remove undefined values and keep the other types intact? I like having Dates everywhere in my application and do the conversion to string right in the API Layer but this behaviour currently messes with that workflow.

What are your thoughts on this? Is this the desired behaviour or could we improve on it?

Thank your very much!

Environment

• React-admin version: latest (5.5.2)
• Last version that did not exhibit the issue (if applicable): -
• React version: -
• Browser: -
• Stack trace (in case of a JS error): -

[fzaninotto]

Thanks for your question.

We use react-query for API calls. API responses use JSON, and therefore when react-admin receives a date from the backend, it is always serialized as a string. And when react-admin sends a date, it is also serialized.

The optimistic update follows this principle.

I don't see any way around this architecture limitation.
If you want to manipulate dates, it must be after the data provider (e.g. in a form transform function).

[kafendt]

Hey @fzaninotto,

thank you for your quick response.
So do I understand you correctly, that react-admin assumes that the data providers return raw json?
I am currently using it in such a way, that the transformation to Date already happens inside my generated OpenAPI Client, so basically inside the data provider.

This also means that every form needs to output string instead of Date right?
The MUI-X Date Pickers return Date however so one would need to make sure to always transform the result before it leaves the form right?
I see you are also using these pickers in the enterprise edition. Is that conversion happening there automatically?

If I know that this is the intended way, I can work with that. But I think this is a problem others might also be running into, as it is not quite obvious to know, that my form can never output Date if I don't want to run into unexpected behaviour.

I do feel like replacing the line:

   const clonedData = JSON.parse(JSON.stringify(data));

with something that simply removes undefined more explicitly could cover both use cases without imposing the restrictions it currently does. But I guess it would also be a breaking change.

In any way I think it might be beneficial to make this assumption more clear in the data provider documentation, as, as far as I know, quite a few OpenAPI or RPC Generators like to transform Date fields to javascript Date just before calling the API / after receiving a response.

What do you think? I am also happy to open a pull request.

[djhi]

So do I understand you correctly, that react-admin assumes that the data providers return raw json?

No, react-admin is backend agnostic. However we do have some defaults on the DateInput you can override with the parse and format props.

This also means that every form needs to output string instead of Date right?

This is the default behavior of HTML date inputs, yes.

The MUI-X Date Pickers return Date however so one would need to make sure to always transform the result before it leaves the form right?

It depends on your dataProvider

I see you are also using these pickers in the enterprise edition. Is that conversion happening there automatically?

We do, by default. Those inputs can leverage parse and format too though.

I do feel like replacing the line:
const clonedData = JSON.parse(JSON.stringify(data));
with something that simply removes undefined more explicitly could cover both use cases without imposing the restrictions it currently does. But I guess it would also be a breaking change.

This is a good point: the conversion to JSON will indeed incur a conversion of dates to string. In that sense, we do impose it for no reason. @fzaninotto or @slax57 what do you think?

[fzaninotto]

The purpose of this line

react-admin/packages/ra-core/src/dataProvider/useUpdate.ts

Line 127 in 107db17

const clonedData = JSON.parse(JSON.stringify(data));

Is to remove undefined values. If you can find a better way to do it without serializing dates, it's not a breaking change, and I agree that it makes useUpdate more backend agnostic.

That being said, the check for undefined values must be done for all values, even nested ones. We'll welcome a PR for this change.