search

poteto / ember-changeset

Ember.js flavored changesets, inspired by Ecto
http://bit.ly/ember-changeset-demo
MIT License
431 stars 142 forks source link

property change notification too broad #499

Open basz opened 4 years ago

basz commented 4 years ago

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 commented 4 years ago

@basz Happen to have more information?

basz commented 4 years ago

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 commented 4 years ago

Is this something you can work with?

snewcomer commented 4 years ago

@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 commented 4 years ago

no change...

snewcomer commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

Hello from #437

snewcomer commented 4 years ago

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

betocantu93 commented 3 years ago

@basz I think this PR https://github.com/poteto/ember-changeset/pull/586 solves this, just tried your repo with it and it works, not sure if its the right approach, waiting for review

basz commented 3 years ago

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

betocantu93 commented 3 years ago

https://github.com/basz/ember-form/pull/7 updated the demo for easier testing

snewcomer commented 3 years ago

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.