property change notification too broad

[basz]

Hi,

I'm noticing that changing one property path also triggers updates to other sibling properties.

a setter (or didReceivedAttrs) is triggered for components like so when a different property in the changeset is changed...

<MyComponent @value={{changeset-get this.changeset "aProperty"}}/>

ember-changeset@^3.5.3
validated-changeset@~0.6.5

[snewcomer]

@basz Happen to have more information?

[basz]

Checkout: https://github.com/basz/ember-form which shows a console message when you click any of the buttons and change the changeset...

I just noticed it happens on classic components only. If I switch over to glimmer components the described behavior does not happen...

Edit: Just realized that set value not being trigger is the correct and expected behavior with glimmer components... The issue remains though, setting a changeset property shouldn't trigger notifications for other changeset properties...

[basz]

Is this something you can work with?

[snewcomer]

@basz Mind trying out the latest 3.5.7 if you don't mind and let me know if this is still an issue?

[basz]

no change...

[snewcomer]

I'm not sure I see an issue in this specific repo. Glimmer components use this.args && @value. Old reactivity paradigms like didReceiveAttrs and computed (push based system) may be at odds with @tracked and a pull based system.

The main thing here is that the changeset has the property you set. If I do a bit of refactoring it looks like it does.

Let me know if you have any other thoughts!

[basz]

I don’t know enough of the internals of push and pull meganisms to make a good argument.

However I do see components are getting values set even when the underlaying value was not interacted with... In both old and glimmer components (also via render modifiers). I have a feeling because of the setProperty trigger inside the proxy. But again I am over my head here...

this[CHANGES] is completely overwritten by a new object; this[CHANGES][key] = change would feel more natural... But again even don't know if that's related at all. (tried it, didn't work :-))

would you know of a way to enable debugging on these low-level glimmer stuff, so we can see when a property is marked for an update?

Anyway... The problem I have is that I have a bunch of BsBootstrap FormElements in a form that all get value updates if one of the control changes. Not a big issue in most situations, but sometimes a control element is constructed from several other controls and their initial state is updated and rerendered. For example, I have a control that shows an image... That image is currently reloaded and results in a visual blink. Not a big deal, but still...

(no critique here on my part. I love this package)

[snewcomer]

My first inclination would be to ensure BsForm is migrated completed to new glimmer semantics. That means removing things like didReceiveAttrs (arguments to glimmer components are now autotracked) and computed. Where I (and others) have been getting in trouble is mixing these two paradigms.

https://guides.emberjs.com/release/upgrading/current-edition/glimmer-components/#toc_updating-component-state

[basz]

Yes that the plan for Ember bootstrap. Might be a while before that’s done...

However the said behavior is also true for glimmer components. I’ve update the demo repo from above to use glimmer component also (args.value and did-insert/did-update).

[snewcomer]

That looks like a better example! Thank you.

I think #492 captures how we need to refine our autotracking abilities. Specifically tracked-built-ins. Basically b/c we reset this[CHANGES] the whole tree of components/props have their cache invalidated. Luckily this just involves reading the same property; however, I see how we could be more efficient! I'll work on tracked-built-ins sometime this week!

[mwpastore]

Hello from #437

[snewcomer]

Reopen to see if we can use tracked-built-ins

[betocantu93]

@basz I think this PR #586 solves this, just tried your repo with it and it works, not sure if its the right approach, waiting for review

[basz]

I’ve learned to live with it, but I can surely appreciate a fix. Thanks! Hope it al makes sense and gets merged.

[betocantu93]

basz/ember-form#7 updated the demo for easier testing

[snewcomer]

I'm all for merging #586. notifyPropertyChange on a nested path is in fact wrong and shouldn't work as it is per-object observability. The PR fixes KVO observability!

Just a few minor questions to address in that PR and we can merge. With the node 10 removal, we would have to make a major bump. So if we could push to another PR, then great. Otherwise, I'm ok merging it all at once.