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() do not catch modifications done with set() method #1512

Closed
Strate opened this issue Apr 21, 2018 · 2 comments
Closed

observe() do not catch modifications done with set() method #1512

Strate opened this issue Apr 21, 2018 · 2 comments

Comments

@Strate
Copy link
Contributor

Strate commented Apr 21, 2018

observe(object, listener) do not catch modifications to existing props of object done with set. It only catches new property added. See jsfiddle: https://jsfiddle.net/4j6b3srp/

const a = mobx.extendObservable({}, {}, {}, {deep: false})

mobx.observe(a, console.log)

/*
logs {
  type: "add",
  object: {…},
  name: "prop1",
  newValue: "new prop"
}
as expected
*/
mobx.set(a, "prop1", "new prop") 

/*
expected to log {
  type: "update",
  object: {…},
  oldValue: "new prop",
  name: "prop1",
  newValue: "changed with set()"
}
but log do not happens
*/
mobx.set(a, "prop1", "changed with set()") 

/*
logs {
  type: "update",
  object: {…},
  oldValue: "changed with set()",
  name: "prop1",
  newValue: "changed with setter"
}
as expected
*/
a.prop1 = "changed with setter"

I think intercept is also affected

@quanganhtran
Copy link
Member

quanganhtran commented Apr 22, 2018

I took a quick look at

const adm = ((obj as any) as IIsObservableObject).$mobx
const existingObservable = adm.values[key]
if (existingObservable) {
existingObservable.set(value)
} else {
defineObservableProperty(obj, key, value, adm.defaultEnhancer)
}

The first time when a new property is set, defineObservableProperty is called and will notify all listeners bound to obj, but subsequent calls to set get out the existingObservable which is an observable value with its own listeners.

Possible solution might be to set the new value through the parent observable object? In that case notification will go through for both the object and the value. Could make a PR if this sounds okay.

@mweststrate
Copy link
Member

MR is merged and will be part of next release. Thanks @quanganhtran

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

No branches or pull requests

3 participants