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

observe-sequence has a bug when _ids have a period #425

Open
znewsham opened this issue Aug 4, 2023 · 11 comments · Fixed by #426
Open

observe-sequence has a bug when _ids have a period #425

znewsham opened this issue Aug 4, 2023 · 11 comments · Fixed by #426

Comments

@znewsham
Copy link
Contributor

znewsham commented Aug 4, 2023

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.

@jankapunkt
Copy link
Collaborator

Hi @znewsham to which Blaze version did you upgrade?

@znewsham
Copy link
Contributor Author

znewsham commented Aug 4, 2023

blaze@2.6.1

but probably more importantly

observe-sequence@1.0.20

@jankapunkt
Copy link
Collaborator

Is it fixed if you upgrade to Blaze @2.71 and observe-sequence@1.0.21

@znewsham
Copy link
Contributor Author

znewsham commented Aug 4, 2023

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

@jankapunkt
Copy link
Collaborator

jankapunkt commented Aug 4, 2023

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

@znewsham
Copy link
Contributor Author

znewsham commented Aug 4, 2023

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
Copy link
Contributor Author

znewsham commented Aug 4, 2023

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

@jankapunkt
Copy link
Collaborator

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

@jankapunkt
Copy link
Collaborator

@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
Copy link
Collaborator

@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

@znewsham
Copy link
Contributor Author

znewsham commented Dec 4, 2023

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

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

Successfully merging a pull request may close this issue.

2 participants