blaze icon indicating copy to clipboard operation
blaze copied to clipboard

observe-sequence has a bug when _ids have a period

Open znewsham opened this issue 2 years ago • 11 comments

I'm in the process of upgrading an app from 2.12 and I'm seeing a reactivity issue with #each - specifically, when an array has _id with a period, e.g.,

[{ _id: 'a.b.c', property: 'whatever' }, { _id: 'b.c.d', property: 'whatever' }]

The contents of the #each block doesn't re-render when the property changes. The cause for this is the change from _.has to a custom has implementation here: https://github.com/meteor/blaze/blob/master/packages/observe-sequence/observe_sequence.js#L361C5-L361C5

Because it does a recursive lookup (on an object that as far as I can tell is always flat) it doesn't see this property and therefore believes the old array didn't have this item.

This would be "ok" if the added callbacks fired, but they don't. What this means is the block re-renders with the previous object.

znewsham avatar Aug 04 '23 15:08 znewsham

Hi @znewsham to which Blaze version did you upgrade?

jankapunkt avatar Aug 04 '23 16:08 jankapunkt

[email protected]

but probably more importantly

[email protected]

znewsham avatar Aug 04 '23 16:08 znewsham

Is it fixed if you upgrade to Blaze @2.71 and [email protected]

jankapunkt avatar Aug 04 '23 16:08 jankapunkt

It doesn't look like it is - the problem is with the has function, which doesn't change between those two versions. In short, I don't think has should be used when looking at posOld - it should also be fairly easy to reproduce

znewsham avatar Aug 04 '23 16:08 znewsham

You're right, I had a similar issue with non-reactive #each blocks in 2.7.0 but they were related to another cause.

The has is actually simply the replacement for _.has as taken from here: https://you-dont-need.github.io/You-Dont-Need-Lodash-Underscore/#/?id=_has

However, since _.has worked before, we should at least check how it works (https://underscorejs.org/docs/modules/has.html) and update the has function accordingly. What do you think?

Edit: we should also update the tests accordingly as it seems this regression was never catched by the tests somehow

jankapunkt avatar Aug 04 '23 16:08 jankapunkt

The difference is, _.has doesn't expand a.b.c into ['a', 'b', 'c'] - so just like _.get - if you want to recurse, you have to split the string yourself. so _.has(obj, 'a.b.c') is equivalent to Object.hasOwnProperty.call(obj, 'a.b.c')

znewsham avatar Aug 04 '23 17:08 znewsham

It looks like https://you-dont-need.github.io/You-Dont-Need-Lodash-Underscore/#/?id=_has is wrong - or maybe referencing a specific version?

znewsham avatar Aug 04 '23 17:08 znewsham

I think it's neither. I really think it's extended but not documented as such

jankapunkt avatar Aug 04 '23 17:08 jankapunkt

@znewsham do you mind helping out in #426 ? We need confirmation that the fix does remove this issue but does not introduce any side-effects

jankapunkt avatar Aug 05 '23 07:08 jankapunkt

@znewsham fiendly ping. If you can confirm this basically running I will write one or two tests and pushb this to the release queue. If you find it hard to test we could also to an RC release

jankapunkt avatar Dec 04 '23 08:12 jankapunkt

Confirmed - it's working, we've had it running in production (a busy site) for at least 3 months at this point

znewsham avatar Dec 04 '23 14:12 znewsham