Fix reactivity for non-primitives

[jankapunkt]

This fixes #417 to make nested data reactive again

[radekmie]

So there are a couple of things:

1. The example you posted on Slack is, in my opinion, incorrect. I never thought that ReactiveVar#set will trigger when I set the exact same non-primitive value in there. I see it's documented, but that's at least unexpected.
2. This code can be simplified in two steps:
   • Replace undefined with { value: undefined } in new ReactiveVar in Blaze._attachBindingsToView.
   • Change x && y ? x.error === y.error && x.value === y.value : x === y to ReactiveVar._isEqual(x.error, y.error) && ReactiveVar._isEqual(x.value, y.value).
3. We should add a test for that (maybe in a subsequent PR).
4. We should copy the example from Slack to here, as otherwise it'll disappear in 3 months.

[jankapunkt]

The example you posted on Slack is, in my opinion, incorrect. I never thought that ReactiveVar#set will trigger when I set the exact same non-primitive value in there. I see it's documented, but that's at least unexpected.

Incorrect in terms of that nested reactivity doesn't really exist with ReactiveVar? Then yes, it should be renamed to "fix reactivity for non-primitives"

This code can be simplified in two steps:

I can add this to the PR

We should add a test for that (maybe in a subsequent PR).

I can also add a test to this PR, I think it should not be that much additional effort

We should copy the example from Slack to here, as otherwise it'll disappear in 3 months.

The example is already in the referenced issue #418

[radekmie]

The example you posted on Slack is, in my opinion, incorrect. I never thought that ReactiveVar#set will trigger when I set the exact same non-primitive value in there. I see it's documented, but that's at least unexpected.

Incorrect in terms of that nested reactivity doesn't really exist with ReactiveVar? Then yes, it should be renamed to "fix reactivity for non-primitives"

Well, yes, changing the name is fine. My thought was that I find it unintuitive that there's more logic than === 🤷

This code can be simplified in two steps:

I can add this to the PR

Great, thank you!

We should add a test for that (maybe in a subsequent PR).

I can also add a test to this PR, I think it should not be that much additional effort

That would be awesome.

We should copy the example from Slack to here, as otherwise it'll disappear in 3 months.

The example is already in the referenced issue #418

Yeah, my bad, I checked the notifications in the reversed order.

[Grubba27]

tests need to be updated 🤔

[jankapunkt]

Yes, I think the latest commit broke things. I'll check later and update.

[jankapunkt]

The tests started failing after I changed

view._scopeBindings[name] = new ReactiveVar({ value: undefined }, _isEqualBinding);

it seems that at some other places the scope binding is not expecting this structure, mainly in all async tests. I can check if we can fix this, however, I would propose to revert to the previous commit, which worked stable.

[radekmie]

it seems that at some other places the scope binding is not expecting this structure, mainly in all async tests. I can check if we can fix this, however, I would propose to revert to the previous commit, which worked stable.

I think it's something different, but this can be handled later. Let's ship this fix!

[Grubba27]

If everyone is on board! will launch this patch

[jankapunkt]

@Grubba27 go for it 👍

[Grubba27]

Blaze & spacebars are out 🚀