Use getTime() to check for Date equality

[davetapley]

Bug report

A new snapshot is created for the same Date.getTime() but different Date object.

Sandbox link or minimal reproduction code

https://github.com/JEFuller/mobx-state-tree/blob/7d8c20321959e51a0607b4a524f407b400a7a92b/packages/mobx-state-tree/__tests__/core/date.test.ts#L47-L57

Describe the expected behavior

MobX-State-Tree is smart enough to know two Date are the same because:

1. This is almost certainly what the user wants, and:

2. The value in the JSONPatch is stored as the getTime() anyway
   (so, you can end up with multiple identical replace patches, which doesn't make sense semantically).

Describe the observed behavior

MobX-State-Tree creates a new snapshot every time.

[davetapley]

I have a test repro, and I'm happy to do a PR to fix, but could do with a pointer where in code to look 😁

[davetapley]

I did some more digging and I'm guessing this would (ideally) have to be fixed upstream in MobX?

• Use getTime() to check for Date equality mobx#3757

[coolsoftwaretyler]

Hey @davetapley - sorry I didn't see this, I've been a bit busy. Will get back to you this evening!

[coolsoftwaretyler]

@davetapley - I'm reading through the MobX issue you opened and I'm somewhat inclined to agree with their interpretation.

I think it's a fair point that two Date objects representing the same point in time are philosophically the same in the real world, but if we think about Dates as objects, much like arrays, I think the JavaScript expectation here is that a distinct date object even with the same time representation is "different" and has changed.

If we have an array: [1, 2, 3] and replace it in MST with a new array with the same contents, [1, 2, 3], MST will consider that a state change.

To keep things consistent, a date object representing 2023-09-17T00:00:00 that gets replaced with another date object representing that same point in time should technically be considered a state change, and trigger a snapshot.

I think this is more about being consistent with JS behavior than the philosophical question of "are these two date instances the same?"

What do you think? I see the test case you wrote for a reprint (thank you, by the way). But I'm curious if you've got a real world problem that could be solved if we used deep equality on this one with getTime(). Would love to help you out if there's some specific pain point. Perhaps we can find some other mechanism (like a computed view that gets cached on the actual time value) to solve your problem.

[davetapley]

Thanks for the reply, obviously happy to defer to maintainers, but r.e. JS behavior vs philosophical question I'll say this ⬇️ and then let it go 😁

MST (and libraries in general) exist because they make solving a problem with a
language simpler, so I'm always a little ambivalent to this kind of 'appeal to purity'.

From my perspective as a user of MST:

1. The existing behavior is already inconsistent (i.e. snapshot or not depends whether assigning a primitive or not).
2. I honestly can't think of a case where assignment causing a consecutive identical snapshots would be the expected or useful behavior (unless I already know how things work under the hood 😉).

---

Glad you mentioned arrays, because I'd extend my proposal to those as well.

Case in point:

If we have an array: [1, 2, 3] and replace it in MST with a new array with the same contents, [1, 2, 3], MST will consider that a state change.

Assuming this test I just wrote is valid, I think that is not correct:

  .create({todos: [1, 2, 3, 4]})
...

    store.todos[0] = 1
    expect(snapshots.length).toEqual(0)

    store.todos.replace([1, 2, 3, 4])
    expect(snapshots.length).toEqual(0)

    store.todos[0] = 2
    expect(snapshots.length).toEqual(1)

Of course, if the array contains non-primitives (e.g. a Date), then we'd get a snapshot every time.

I wrote another test and we can see that whether a snapshot happens depends on exactly how you update something:

    const a = Task.create({ x: "a" }),
        b = Task.create({ x: "b" }),
        c = Task.create({ x: "c" }),
        d = Task.create({ x: "d" })
    const store = types
        .model({todos: types.array(Task)})
        .create({todos: [a, b, c, d]})


    store.todos[0].x = "a"
    expect(snapshots.length).toEqual(0)

    store.todos[0] = { x: "a" }
    expect(snapshots.length).toEqual(1)

    store.todos.replace([a, b, c, d])
    expect(snapshots.length).toEqual(2)

Again, all makes sense assuming you know how it works under the hood, but not immediately obvious.

---

Finally (sorry this got so long!):

My real world problem happened because I poll an API in the background of my app to get updates.
It'd been working great, but then I added a Date to my model and the performance suddenly was terrible 📉
After much digging I realized it was now triggering a re-render of 1000s of component every five seconds, even though 'nothing' (in the 'real world' sense) had changed.

I do have a workaround, and you figured it too:

a computed view that gets cached on the actual time value

It isn't that bad, since the time stamp comes from the API is an 8601 string I can just put that in my model and use a view. But it took while to figure out what was going on and why.

[coolsoftwaretyler]

@davetapley - whoa! Excellent detail here, and I was totally wrong about the array replacement. Thank you for the correction. I haven't had a chance to run your tests, but I wrote up a code sandbox here. I make this mistake all the time, and to be honest, a lot of the times when I write MST code, I'm doing a little guess-and-check, so I just had the totally wrong mental model on this. I appreciate your pushback.

I also apologize if I came off as putting purity over practicality, I legitimately thought our existing behavior was more consistent than it seems to be. I care a little less about the purity, and more about the consistency. But if there's inconsistency already, then I'm less concerned on that front.

All that to say, I'm somewhat convinced that we might want to consider making this change to how the date equality is determined, but there are two major blockers:

1. I still think this might be coming from MobX and not MobX-State-Tree. In which case, we may have our hands tied. I would not want to subvert expectations by modifying their behavior on our end.
2. I think the change you are suggesting would be a breaking change. This isn't a reason not to do it, but it's a reason that we might not get around to it for a long time. We have a lot of outstanding issues and table stakes work to do in this repository to get things back on their feet, and breaking changes are necessarily at the bottom of the priorities.

Thank you for describing the real-world behavior you experienced. That also helps suggest where the pain point is and how we might write some tests to fix this.

I don't think I can put this on top of my priorities. I'd be happy to review a PR to change the behavior, but I'd also want to be clear at the outset that I think it's a breaking change and might get de-prioritized, and it's possible that other folks in the community would ask for us to not ship the change based on other objections. So I am not ready to say yet if I would rubber-stamp this through.

If it's cool with you, how about we leave this issue, I'll tag it with some labels for further consideration, and we can see where it goes? I'm sorry if that's not a very satisfying answer at the moment, but I'm very happy to keep chatting about it.

[davetapley]

@coolsoftwaretyler that all sounds good ✅

As an aside (and for future record), have you seen / used comparer.structural mobxjs/mobx#3757 (comment)?

It looks like a MobX feature which gets closer to what I'm proposing, but no mention of it in MST.

[coolsoftwaretyler]

Hey @davetapley - I can't say I have! Seems neat. I don't drop down outside of the MST API too often myself, but I'm hoping to get into the internals more over the coming months.