[Optimization] Don't trigger observers if prev value and new value are same

[SukantGujar]

For this snippet -

const observer  = require("@risingstack/nx-observe");

let context = observer.observable({"name" : "a"});

const signal = observer.observe(()=>console.log(context.name));

let name = context.name;

context.name = "a";

The observer is triggered twice. Wouldn't it help to not trigger observers if the value hasn't changed?

[solkimicreb]

You just found some deep stuff, that needs some explanation (:

Observed functions always run once synchronously after you pass them to observe(). This is needed to expose a predictable synchronous behavior (similarly to mobx.)

After that observer functions are never triggered synchronously on changes, but collected in a Set instead. This set removes the duplicates over time and after the stack empties (the current batch of synchronous code finishes) all the triggered observed functions are executed (without duplicates). This ensures that no observed function runs more than once per stack. The following would only run once and it would print 'c' to the console.

let context = observer.observable({"name" : "a"});

context.name = "a";
context.name = "b";
context.name = "c";

This is preferred by most observable libraries (not by mobx though, but it has 'transactions' and 'actions' instead).

Your example

In your example you registered a new observed function, which caused it to run synchronously. Then you triggered it with an observable change, that made it run after the current stack emptied. It is the only situation when an observed function can run more than once per stack. And even in this situation it runs max twice.

About not triggering on same value

This would be nice but it causes some problems with built in objects (like Array, String, etc). Built-ins have internal implementation which should be intercepted by Proxies, but it is still buggy in some cases (these are being fixed over time). One of these bugs is with Arrays in v8. When an array length is modified by some native array method (like push) the internal code does length = length + 1, which is not intercepted by Proxies, and the external interceptible code does length = length somewhy. This bug is the main reason why I listen on non value changing operations too. In the future it can be removed.

I hope it made sense. Sorry for the long comment and thx for the issue.

[solkimicreb]

I could modify things in two ways to ensure that an observer always runs once per stack and no more.

1, A single line change. Change this line to queueObserver(observer). This would run the observer asynchronously ONCE after its registration.

OR

2, A few lines change. Do not queue observers in the stack in which they were registered (passed to observe()). This would run the observer synchronously ONCE after its registration.

If it is needed I would go with option 2, as having a synchronous first run can be nice.

[SukantGujar]

I definitely love the deferred update approach nx-observe takes. It itself removes the possibility of duplicate render calls in my code. And I do see the benefits of synchronous first call although for my React app, its not exactly a requirement. At the moment the current behavior is causing non-determinism with observer callbacks in these situations...

context.name = "a";
context.name = "a";
context.name = "a";

Output:

a
a

context.name = "a";
context.name = "a";
context.name = "b";

Output:

a
b

Would the second option you mentioned above fix this?

[solkimicreb]

I agree that this needs some improvement, but I am not sure I get it ): The two examples you provided are different. It would be non deterministic if it resulted in different behavior for the same input. For the first one it will always be a,a and for the second it will always be a,b.

• The 1. proposed fix would turn the results into 'a' for the first example and 'b' for the second.
• The 2. proposed fix would turn the results into 'a' for the first example and also 'a' for the second.

I also have a 3rd proposal.

Add a config to nx-observe with a trigger options which could be 'sync', 'async' or 'sync-once'.

• 'sync-once' would result in the current behavior. Observers are usually executed once per stack, sometimes twice.
• 'async' would result in an always async behavior. Observers are always executed once per stack.
• 'sync' would result in an always sync behavior. Observers can be executed a lot of times per stack (this should be sued with care as it can lead to infinite loops and other nasty things).

This would require only a few lines change (although i am not yet sure what API to provide for the config). What's your opinion?

Off topic: are you trying to get React working with nx-observe?

Edit: after some thinking, the second approach (running once synchronously and no async call in that batch) is bad. It would not react properly to changes in the first stack. So option 1 and 3 remains. I think I like 3 a bit more.

[SukantGujar]

Yes, I am experimenting with nx-observe in a React/React-Native app. In the two examples I gave, the expected output is a for the first and a, b for the second example. The reason is that from the application state point of view, in the first example the value of the name field never changes after the initial assignment. For the second example, it changes from a to b across three calls. So my React view should initially render displaying a and then only re-render to display b. However, with the current code, the render calls are multiple even with same values.

[SukantGujar]

My current implementation is based on ImmutableJS where I have the luxury of easy comparing previous instance value with current instance value and invoke render if I detect a change. I was hoping it should behave similarly here. From purely state updates perspective, the observers should be invoked when a true change is detected between previous and current values.

[solkimicreb]

Cool (: I am glad you are experimenting with nx and React.

I think that in case of React (or any client side code) the optimal result would be a for the first ca se and b for the second. Triggering for a in the second example would run the render function for a and shortly after it would run the render function for b again with no paint events in between. This means an extra render without any user facing effect.

Having synchronous reactions could make sense on the server side though, so I will go with option 3 I think and add a config with the mentioned options.

Is it okay if I make it global, like nx.config({ trigger: 'async' })? Or should it be toggleable per observer? I think mixing sync and async trigger modes is not a really needed feature and it would juts cause troubles.

Edit: The trigger: 'async' option would result in the 'expected' result for your use case. (Meaning minimal reruns.)

[SukantGujar]

Sorry for the simple example. I just realized that the behavior was because my simple code screwed it up by putting everything in the same execution context 😝 . Ok, so after I changed the sample code to use timeouts, its throwing up less observer calls. However there are still some for the same state value, but I think you have been trying to explain ways to fix this behavior only to me all this long :). The following code -

setTimeout(()=>context.name = "a", 100);
setTimeout(()=>context.name = "b", 1000);
setTimeout(()=>context.name = "c", 2000);

Gives this:

a
a <- not needed
b
c

[SukantGujar]

Okay probably I am having a really long day. But after trying out with more combinations, in async mode, the observers are invoked always, even when the new value is exactly the same as the old one. Here's the complete snippet just to ensure I haven't screwed it again -

const observer  = require("@risingstack/nx-observe");

let context = observer.observable({"name" : "a"});

const signal = observer.observe(()=>console.log(context.name));

setTimeout(()=>context.name = "a", 100);
setTimeout(()=>context.name = "a", 500);
setTimeout(()=>context.name = "a", 1000);
setTimeout(()=>context.name = "b", 2000);
setTimeout(()=>context.name = "c", 3000);

The output is -

a
a <- not needed
a <- not needed
a <- not needed
b
c

[solkimicreb]

Yep, I understand the problem now :D I will try to clarify things a bit. Longer comment is coming in a moment.

[solkimicreb]

The async rerun is needed here because of a bug with V8. Some built-in objects (like String, Date, Array, etc) have native bugs with Proxies. The reason is that these were implemented with native code before, that could not be intercepted by JS. Then Proxies were introduced which should intercept property access inside these internal objects and functions.

The particular bug that makes listening on non value changing property mutation necessary is an Array bug. Native Array mutation methods (like push) change the array length in a non interceptible way, while they also change the length to the same value in an interceptible way.

This is what array.push does internally.

// bugged interceptible code
array.length = array.length

// native, but correct non-interceptible code
array.length = array.length + 1

This makes it necessary to listen for array.length = array.length like changes. For example the following code would probably not be observed correctly without it.

let context = observer.observable(['item1'])

observer.observe(() => console.log(context.join(', ')))

// this would not trigger the log
context.push('item2')

[solkimicreb]

I am not sure about this though since the push should also add a new key for the new array item. (1 in case of the previous example), which should be picked up by NX. I will look into this.

Removing triggers on same value mutations would be a small change, I just want to be sure that it wont expose any native bugs. I think I will add tests for some common built-in objects (Regexp, String and Array for a start)

[SukantGujar]

Got it, and thanks for the in-depth explanation. There are easy workarounds of the multiple renders for me, so this won't be a blocker :). As this only impacts value types, a simple shallow comparison in my React component will take care of multiple re-renders.

Should I close this issue?

[solkimicreb]

Not yet please, it is a valid issue and it would be optimal to fix it inside nx-observe instead of workarounds. I will look into it and if it is doable I will link the PR here.

Thanks again for the issue (:

[solkimicreb]

Hi!

This PR fixes all of your issues. It needs a little more work but it will be merged soon.

For your use case you will want to use this config:

const observer  = require('@risingstack/nx-observe')

observer.config({ mode: 'async', alwaysTrigger: false })

mode: 'async' disables the first synchronous run on observed function registration. alwaysTrigger: false will cause NX to not trigger observed functions on observable property changes that resulted in no value change.

(Config prop names may change a bit to be more descriptive)

[SukantGujar]

Thanks, will give it a run today.

[SukantGujar]

I tested the PR today. It's only reporting a single change in my test code...

[solkimicreb]

Ouch, thx. Looking into it. I will also add some extra tests.

[solkimicreb]

Fixed and merged.

From version 2.0.5 set operations without a value change will not trigger observers. This version also fixes built-in object support (Set, Map, WeakSet, WeakMap, Date, String, etc). Tests and readMe is also updated.

First synchronous run will still happen and the config function didn't make it (caused more trouble then good).

I am closing the issue, we can discuss async mode in another issue later maybe.

[SukantGujar]

Howdy? Another off topic question :). Being inspired with ES6 proxies through nx-observe, I am planning to implement a JSON Schema validator of object models via a Proxy. Let's say the ORM model-instance creation looks something like -

let car = Car.of(carPOJO)

Validations could be done like -

let validationError = car.model.isValid()

Internally, the Car.of method wraps carPOJO into a Proxy which adds a get trap and listens for isValid and would then use the object model to either return true or validation errors. A cool thing would be that I would then wrap car inside nx-observe and would have observables of my validatable instances!

Thoughts, suggestions, concerns!?

Do note that I am beginning with Proxies and don't know of any edge cases you have seen in the wild.

Thanks in advance!

[solkimicreb]

I am not sure I understand, especially this part: Proxy which adds a get trap and listens for isValid. Can you provide an example snippet or maybe a bit more detail?

Thx (:

Edit: did you manage to get nx-observe working with React? I am really interested (:

[SukantGujar]

Unfortunately, I am still getting only the first change notification with the below code on 2.0.5 -

const observer  = require("@risingstack/nx-observe");

let context = observer.observable({"person" : {name:"a"}});

const signal = observer.observe(()=>console.log(context.person));

setTimeout(()=>context.person.name = "a", 100);
setTimeout(()=>context.person.name = "a", 500);
setTimeout(()=>context.person.name = "a", 1000);
setTimeout(()=>context.person.name = "b", 2000);
setTimeout(()=>context.person.name = "c", 3000);

Output:

{ name: 'a' }

Other than that, we have been successful in integrating nx-observe with React. I will share some snippets when the code is more presentable :).

[SukantGujar]

Quick update, changing the observed function callback to the below gives me the desired result - const signal = observer.observe(()=>console.log(context.person.name));

Output:

a
b
c

I guess the previous code was tracking parent person object and wasn't notified when the child property name changed. This is expected isn't it? Considering that the parent object reference hasn't changed.

[SukantGujar]

On the off-topic topic :)... long post warning! The ORM I am working on is supposed to validate data against json schema. The use case would typically involve the user defining a JSON Schema for data-models in an editor. Then my application will compile this schema into an ORM Model code which will have the capability to validate POJO objects against the schema. This idea is based on this concept. So from Car json schema, I create a Car ORM type. Most common validation use case is to validate in the UI as the user edits an instance. E.g. on a hypothetical Add a new car in your garage form, the user will be adding data which will be used to create a new Car instance and store it in the back-end. Validations errors are typically shown as early as possible. So with every form field, I need to store the schema of the corresponding ORM field and then validate the user input against that. Something like -

<Text field={car.color} 
   fieldType="Car.color" 
   label="Color"
   placeholder="Color of your car..."/>

where car is an instance of Car type. This way the Text component can use Car.color schema information to do the validation in onchange, using an API like car.color = rgb(this.value); validate(Car.color, car.color).

Instead of towing the schema of my ORM type alongside the value in each component, I was initially thinking of storing the schema inside each instance property in Symbols, so that they are hidden from general inspections and loops, yet are accessible to the validation API. With this approach, e.g. my validation approach changes to car.color = rgb(this.value); validate(car.color);. The validate method will fetch the hidden color property schema from car.color instance and will validate the value against it. But this won't work on value types where you can't add symbols as there are no objects to add them to :(.

A better approach would be to wrap the schema into a proxy object on the field itself. This way, my original object is not injected with hidden symbols. The Car ORM type code will create and return proxied objects. Every such proxy will have a isValid hidden method which can be invoked. This method will be handled using a get trap by the schema proxy, and will be used to validate the instance value with the stored schema. So my validation logic now changes to - car.color = rgb(this.value); car.color.isValid(). This also has the added benefit of dealing with value types where you cannot add Symbols as the types cannot be extended.

Make sense?

[solkimicreb]

Firstly on the on-topic thing. Yes I also overlooked it (: console.log is a special case. Everything is tracked that is directly get in the observed function. console.log does not 'directly get' the nested properties so the above won't trigger the observed func. But if you try console.log(JSON.stringify(context)) it would trigger it. I am not sure if the console.log thing counts as expected or it is more like a bug, I will think about it.

[solkimicreb]

About the schema validation library. First of all thx for the explanation (:

I think you will have the same issue that you had with symbols in case you use proxies. If you try to wrap a primitive in a proxy you will get this error: Uncaught TypeError: Cannot create proxy with a non-object as target or handler.

Why don't you build a 'symmetrical schema' instead of adding it on a per field level. I mean doing something like this. (It also look much like the joi approach).

const car = {
  color: 'red',
  weight: 2000
}

const carSchema = {
  color: {
     type: 'string',
     ........
  },
  weight: {
    type: 'number',
    .........
  }
}

validate(car, carSchema)

// OR 

validate(car.color, carSchema.color)

[SukantGujar]

Then I will need to tag both schema and the instance across the form fields. But if proxy wrapping doesn't work across primitives then the idea won't work anyways... Oh well, I need to cook up some code and share that with you for a more detailed discussion.

[SukantGujar]

Regarding the notifications on person when its child reference is changed... With normal objects this shouldn't be an issue, but this may pose a challenge in dealing with objects representing a Collection<key, value>, something like -

"candidates" : {
    "first" : {
        "id" : "first",
        "name" : "Amy"
    },
    "second" : {
        "id" : "second",
        "name" : "Lucy"
    }
}

In React, you typically have a parent component bound with the collection, which renders each item as a child component in an iteration inside its render method. Then we make these components re-render when the corresponding observables change. Imagine that the parent is observing the candidates object while every child component is observing its respective candidate entry, e.g. candidate.first, candidate.second and so on. Now in scenarios where the user changes the values in any existing item, they will be rendered correctly because there is an existing child component observing it. However when a new element is added to the collection, the parent component must re-render so that a new child component is added to the DOM, bound with the new element. A stringify could have a performance impact in such cases depending to the data size. A more preferable approach would be to define a calculated size getter in the collection and observe it for changes in the parent component. What do you say?

[solkimicreb]

It would work as expected, no need for workarounds. Every synchronous JS code is correctly observed and I am pretty sure about this. If you would like to render something you need to get it somehow in your render function. And this get will be observed even if it is a very obscure and 'tricky' get operation. The only exception to this are built-in objects which are all fixed to work by now except for console.

console.log is a special case because I really can't decide what should be the 'correct behavior' for it. For example take a look at the following code.

const person = observer.observable({ name: { first: 'John', last: 'Smith' }, gender: 'male' })

// display is a render-to DOM function here
observer.observe(() => {
  display(person.name.first + person.name.last)
  console.log(person)
})

Lets say that the display function has some bugs and does not display the person's name. The developer then adds console.log(person) to see if the issue is with the person fields. Probably the developer would still expect the observed function to rerun only when person.name.first or person.name.last changes. This is true now, but alternatively console.log could work like anything else. In that case the above function would rerun when person.gender changes too.

I think both case are confusing. The reason why I prefer a less reactive approach for console.log is because it has no user-facing effect (it is mainly used for debugging).

EDIT: you were right (: for in loops are not tracked correctly, I will fix it soon (I think I will have to add an extra has trap to the Proxy to cover that case). Other than that iterations (for of, forEach, custom, etc) are all tracked correctly.

[SukantGujar]

Ah, I see. Thanks, will test with a real react render.

[SukantGujar]

Here's another test where I think I'd be getting the first observable invoked thrice, but it gets invoked only once...

const observer  = require("@risingstack/nx-observe");

let trails = [];

let context = observer.observable({"candidates" : {"first" : {"id" : "first", "name":"Amy"}}});

const signal = observer.observe(()=>trails.push(JSON.stringify(context.candidates)));

setTimeout(()=>context.candidates.second = {"id" : "second", "name":"Tracy"}, 100);
setTimeout(()=>context.candidates.third = {"id" : "third", "name":"Joe"}, 500);

setTimeout(()=>console.log(trails), 2000);//<- Only logs first"
setTimeout(()=>console.log(context), 2000); //<- context has all three entries

This example is close to the React collection scenario. The parent's observer function cannot watch for additions of any children and as a catch-all I added stringify. To rule out console.log, I am collecting the changes made to the observable in an array and then finally logging it. Is something amiss?

[solkimicreb]

I see now. Are you sure that this is how React behaves? I think that people would use and array, Map or Set for collections like the above.

const observer  = require("@risingstack/nx-observe");

let context = observer.observable({candidates: [{id: 'first', name: 'Amy'}]})

observer.observe(render)

function render () {
  for (let candidate of context.candidates) {
    display(candidate)
  }
}

// a render-to DOM mock
function display (item) {
  console.log(item)
}

setTimeout(() => context.candidates.push({id: 'second', name: 'Tracy'}), 100)
setTimeout(() => context.candidates.push({id: 'third', name: 'Joe'}), 200)

I will think about this. Thx for all the bug catches (:

EDIT: this logs the following { id: 'first', name: 'Amy' } (first run)

{ id: 'first', name: 'Amy' } (second run) { id: 'second', name: 'Tracy' }

{ id: 'first', name: 'Amy' } (third run) { id: 'second', name: 'Tracy' } { id: 'third', name: 'Joe' }

The rest is up to the vdom diffing.

[solkimicreb]

Just a general tip for the examples: make sure to actually get the resource you would like to use in your examples. NX-observe works by intercepting and registering get operations (just like mobX does). And this leads to a 'correct' behavior.

For example if your previous snippet was a real React render method how would you actually render? You have this: () => trails.push(JSON.stringify(context.candidates)). In order to render the candidates you would have to somehow iterate them, which would cause nx-observe to register them. Your example does not cause nx-observe to register the candidates because it is unnecessary for now, but right after you actually access them and use them for something it will pick them up.

Hope this makes sense.

[solkimicreb]

(The fact that it does no intercept for in loops is a bug though which needs fixing. I suspect that JSON.stringify also uses for in internally so it is probably also bugged). Thx for that catch (:

[SukantGujar]

You guessed it right, the render of the parent will have a map over candidates. But, you wouldn't want the parent to worry when an existing child's state changes, the only operations on which the parent would need to re-map would be addition or deletion of a child.

Think of it this way.... The parent is rendering a lot of child items. If a single change in its children (except new/delete) causes re-render to the parent, it would be very costly. In fact, not having to re-render a heavy component in most of the use cases is a big win of observer backed stores like nx-observer and MobX. In my previous example, a react component's observer code would be something like-

this.setState({"items":candidates})

Notice the setState call, it queues up the render.

This is sort of tricky :). This is why I was thinking of checking on collection's size in the parent's observer. That'll allow me to queue up a re-render on new/delete. A Map is a better alternative than a POJO as you suggested, and I will get size for free :).

[SukantGujar]

Here's how the react components and observables tie in... The black arrows indicate which component is observing which prop -

[solkimicreb]

I am pretty much a React newbie, but I would guess that you can solve it by not putting child logic into the parent render. An example:

class Parent extends Component () {
  constructor (props) {
    super(props)
    this.props = observer.observable(props)
    this.state = observer.observable(this.state)
    observer.observe(this.render, this)
  }

  render () {
      return this.props.items.map(item => <Item key={item.id} item={item}>)
  }
}

class Item extends Component () {
  constructor (props) {
    super(props)
    this.props = observer.observable(props)
    this.state = observer.observable(this.state)
    observer.observe(this.render, this)
  }

  render () {
      return <span>{this.props.item.name}</span>
  }
}

Parent render would not be triggered when an items internal properties change, as it does not get any of these properties. The appropriate child render would trigger as it gets the property needed.

(The above is just some pseudo code. I don't have the knowledge and time yet to make things work with React. nx-observe is primarily designed to be the observer library of the nx-framework and it works there just as you suggested (with minimal number of components being rerendered))

[solkimicreb]

Thx for all the examples and effort you put into this btw (:

[SukantGujar]

That was fast! This is more or less how it's done (although you wouldn't wanna observed render as its invocation is better left on React. We observe another function which uses setState call, which schedules a render). We have seen immediate perf gains with nx-observe. But this won't re render when a new item gets added or deleted if the parent is a simple object. I am gonna try with Map tomorrow and it'd work. Array works by the way 👍.

[solkimicreb]

Great!

In a few hours I will release nx-observe v3.0.0. It fixes the issue with the for in loop, so your previous examples (including the console.log one) should work with it. It also switches to an always async execution (no synchronous first run) and exposes a queue(fn) method that queues the passed function to run together with the triggered observed functions. (I will also start to do proper releases and change logs)

[solkimicreb]

nx-observe 3.0.0 is published. It comes with all of the improvements mentioned above.

The examples you mentioned above has the following result with v3.0.0.

1, code:

const observer  = require("@risingstack/nx-observe");

let context = observer.observable({"person" : {name:"a"}});

const signal = observer.observe(()=>console.log(context.person));

setTimeout(()=>context.person.name = "a", 100);
setTimeout(()=>context.person.name = "a", 500);
setTimeout(()=>context.person.name = "a", 1000);
setTimeout(()=>context.person.name = "b", 2000);
setTimeout(()=>context.person.name = "c", 3000);

output:

{ name: 'a' }
{ name: 'b' }
{ name: 'c' }

2, code:

const observer  = require("@risingstack/nx-observe");

let trails = [];

let context = observer.observable({"candidates" : {"first" : {"id" : "first", "name":"Amy"}}});

const signal = observer.observe(()=>trails.push(JSON.stringify(context.candidates)));

setTimeout(()=>context.candidates.second = {"id" : "second", "name":"Tracy"}, 100);
setTimeout(()=>context.candidates.third = {"id" : "third", "name":"Joe"}, 500);

setTimeout(()=>console.log(trails), 2000);

output:

[ '{"first":{"id":"first","name":"Amy"}}',
  '{"first":{"id":"first","name":"Amy"},"second":{"id":"second","name":"Tracy"}}',
  '{"first":{"id":"first","name":"Amy"},"second":{"id":"second","name":"Tracy"},"third":{"id":"third","name":"Joe"}}' ]

[SukantGujar]

Tested and working great, thanks!

[ippa]

@SukantGujar: How did the work with integrating nx-observe into react/react-native go?

I've found Redux to be overkill for smaller projects. MobX looks better but I think nx-observe could solve it in an even better manner.

[solkimicreb]

@SukantGujar @ippa I made a new issue for React related discussion: https://github.com/RisingStack/nx-observe/issues/6

[SukantGujar]

@ippa So far the integration has been good :), @solkimicreb has been very helpful and has helped us a lot by fixing issues that came along the way. The implementation is very much like with mobx and design considerations are also similar. The project is not open source yet and I have been working on other challenges with it. But I will try to share a simple example soon on the discussion which @solkimicreb spun from this.