applyPatch issue with dates

[ddimitrioglo]

Due to clone's specific behavior: source - should be a JavaScript primitive, Array, or (plain old) Object., it's hard to work with Dates.

1. Doesn't work:

const issue = {
    openedBy: 'ddimitrioglo'
};

applyPatch(issue, [{ op: 'add', path: '/openedAt': value: new Date() }])

>> { openedBy: "ddimitrioglo", openedAt: Object {} }

2. Doesn't work:

const issue = {
    openedBy: 'ddimitrioglo',
    openedAt: undefined,
};

applyPatch(issue, [{ op: 'replace', path: '/openedAt': value: new Date() }])

>> { openedBy: "ddimitrioglo", openedAt: undefined }

3. Works, but it means that I can't have optional properties:

const issue = {
    openedBy: 'ddimitrioglo',
    openedAt: null,
};

applyPatch(issue, [{ op: 'replace', path: '/openedAt': value: new Date() }])

>> { openedBy: "ddimitrioglo", openedAt: <Date> }

Is this behavior important and couldn't be changed for some reason? Is it possible to work-around this?

UPDATE 1

For the sake of curiosity used lodash.cloneDeep and my issue was fixed (without breaking any tests)

export function clone<T extends any>(source: T): T {
  return cloneDeep(source);
}

[chbrown]

First, a partial easy answer: your 2. shouldn't work, per the official spec — note the MissingError in the array returned by applyPatch.

The differing behavior of add (clones value) vs. replace (does not clone value) is inconsistent, for sure. I couldn't at first recall why it ever clones the value, but apparently it's due to #44, which is more thoroughly hashed out at #38. 2+ years on and considering the mutating behavior pervasive throughout the library I'm wondering if it was the right idea to ever clone values attached to operations, but it's probably less confusing to the user experience than not doing so, as evidenced by the issues above... despite getting in the way sometimes, as you've noticed.

Btw the reason why lodash.cloneDeep works is because it spends 600+ lines of code handling a swath of various types, while my naive clone takes 20 LOC to handle just the primitive/Array/Object cases.

Sorry to say, but I think part of the solution is to make your 3. stop working 😢 — i.e., replace should clone value just like the other operations.

More to your point though it might be reasonable to specially treat Date instances as "primitive" within clone; I'll have to think about that a bit further but it's probably preferable over treating it like a POJO (i.e., the current behavior), despite not being semantically accurate.

[ddimitrioglo]

@chbrown thank you for the update.

Yes, looks like treat Date instances as "primitive" within clone is the best solution here (any ETAs on that?).
Also would want to ask you a question: is doing everything by yourself is a matter of principle? why don't you use things that someone already did (like lodash.cloneDeep)?

P.S. Maybe it's worth adding a Contribution section in the README.md and provide a guideline, share your vision on the project, and future plans. That would be helpful for those who want to contribute.

[chbrown]

Fixed on clone-date branch if you wanna help test it out before release (which'll be a patch-level version bump). You can switch to the branch by changing your package.json's dependencies from {"rfc6902": "^4.0.1"} to {"rfc6902": "github:chbrown/rfc6902#clone-date"}

FYI, I changed my mind about the solution — once I got into writing the fix I realized that adding a special case for classifying Dates as "primitive" would introduce practically as much complexity as a special case for properly cloning them, so I implemented the latter.

---

Also would want to ask you a question: is doing everything by yourself is a matter of principle?

Your premise isn't quite right, because I don't "do everything by myself," but let me try to address what I think you're getting at:

• Every external dependency introduces a trade-off between functionality (good) and complexity (neutral/bad).
  • On the upside, if I'd used lodash.cloneDeep from the start you wouldn't have run into this issue.
  • On the downside, I'd have added a lot more complexity (increasing fragility, decreasing debuggability/readability/transparency) & code (increasing distribution size), plus I'd be committing to the notion that lodash.cloneDeep always does the right thing (though, for a library as established as lodash that is probably a safe bet).
• In this case, my aversion to introducing a dependency on lodash.clonedeep and using that instead of my util.clone is that I've judged the trade-off too steep — the functionality doesn't outweigh the complexity.
  • The rfc6902 codebase (without tests) comprises 429 LOC (of TypeScript); lodash.cloneDeep itself is 655 LOC (of JavaScript). That would be a 150% increase.
  • The clone functionality is not integral to the core functionality of this package — i.e., needing to clone values is more an artifact of JavaScript's type system than anything having to do with JSON Patches and Pointers.
  • As I mentioned above, cloning operation values at all is a concession to improve user experience. Which is important, certainly, but substantially reframes how I evaluate the trade-off (vs. if the dependency in question were contributing core functionality).

Maybe it's worth adding a Contribution section [...]

Good point. I should do that -> #79.

[ddimitrioglo]

@chbrown don't get me wrong, I see your point of view and can (should) accept it! just wanted to hear that point and avoid any assumptions 😉

P.S. will check the fix tomorrow and come with updates

[ddimitrioglo]

@chbrown the fix covers all my cases, so looks like it's ready for a new npm version 😉

[chbrown]

https://www.npmjs.com/package/rfc6902/v/4.0.2