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

createPatch does not support Date type #100

Open
hollandjake opened this issue Aug 19, 2024 · 3 comments
Open

createPatch does not support Date type #100

hollandjake opened this issue Aug 19, 2024 · 3 comments

Comments

@hollandjake
Copy link

Following on from #78 , createPatch should have Date handling added so that JS objects with a Date object inside are handled correctly.

Currently they are effectively ignored.

const userA = { createdAt: new Date(2010, 7, 10, 22, 10, 48) };
const userB = { createdAt: new Date(2011, 7, 10, 22, 10, 48) };
const patch = createPatch(userA, userB);
// []

This is because in the current handling of diff, Date objects get read as an object type and so it attempts to iterate over its keys of which there are none, resulting in an empty patch despite the value being different.

@gabynevada
Copy link

Getting the same issue. @chbrown is this possible to be verified or that I can help in to resolve the issue?

@chbrown
Copy link
Owner

chbrown commented Nov 27, 2024

RFC 6902 Section 3 says

A JSON Patch document is a JSON [RFC4627] document that represents an array of objects.

JavaScript's Date type is not a 'thing' in JSON, so I'm not sure what you want the output of createPatch to be. Though I will grant that returning an empty patch in your case is maybe not ideal. Should it throw an error? Should it very opinionatedly call .toISOString() on any Dates it encounters when it's expecting JSON? Should it defy the spec and return a patch that contains a JS Date instance?

There is the "optional diff argument" (search the README) which lets you pick which of those outcomes you want. Maybe there should be a batteries-included module of diff functions that users could import and plug into that argument, to avoid having to write their own.

@hollandjake
Copy link
Author

hollandjake commented Nov 28, 2024

@chbrown good point on the use of the optional diff argument. Although it does seem like since Date has been part of the JS spec for so long and is basically a native type now, I think it should be handled and preserved as its Object form.

If the user so wishes a Date to become something else they can always run JSON.stringify, which will convert it into an ISO string inside the patch.

Of course this has the side effect that if you stringify and parse the patch you will not be able to apply the patch correctly since its not a reversible action. Maybe we transform the Date into SuperClass of Date which has a custom toJSON method resulting in a transformation like:

class PatchableDate extends Date {
  toJSON() {
    return {"@type": "ISODate", "value": this.toISOString()}
  }
}

const a = new PatchableDate("2024-11-28T10:09:08.000Z")
console.log(JSON.stringify(a) === `{"@type":"date","value":"2024-11-28T10:09:08.000Z"}`)

This would allow people to keep using whatever logic they want on a JS Date object.
But this would depend on whether we care to preserve the stringify case or whether that is deemed expected behaviour.


At the very least we should definitely be supporting the comparison case of Dates.

const dateA = new Date("2024-11-28")
const dateB = new Date("2025-12-29")

should result in a patch.

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

No branches or pull requests

3 participants