aurelia / binding

A modern databinding library for JavaScript and HTML.
MIT License
111 stars 96 forks source link

Unset oldValue after being done with it #772

Closed ccantill closed 4 years ago

ccantill commented 4 years ago

fixes #771

bigopon commented 4 years ago

@ccantill thanks. I see what you meant. I'll have a look to see whether we need to be carefully setting initial value of old value to something else or simply null would suffice

bigopon commented 4 years ago

I think this is fine. As the observer always deals with outside using currentValue. We may need this for v2 as well cc @fkleuver @EisenbergEffect

fkleuver commented 4 years ago

Actually I already solved this in v2 (unintentionally) when I was doing perf optimizations: https://github.com/aurelia/aurelia/blob/master/packages/runtime/src/observation/setter-observer.ts#L61

I think we can do it the same way in v1; just change this:

  call() {
    let oldValue = this.oldValue;
    let newValue = this.currentValue;
    this.oldValue = null;

    this.queued = false;

    this.callSubscribers(newValue, oldValue);
  }

To this:

  call() {
    let oldValue = this.oldValue;
    let newValue = this.oldValue = this.currentValue;

    this.queued = false;

    this.callSubscribers(newValue, oldValue);
  }

It won't matter much, but it can be slightly better for perf in otherwise monomorphic hot paths.

bigopon commented 4 years ago

That looks good to me, I was thinking of that too but wouldn't want to bother too much with non-outcome requests 😛 @ccantill can you please help with that?

ccantill commented 4 years ago

I've changed it in my PR.

ccantill commented 4 years ago

@bigopon any idea on when this can be merged/released?

bigopon commented 4 years ago

@ccantill oops, sorry missed this. pinging @fkleuver @EisenbergEffect for a merge & release

fkleuver commented 4 years ago

@ccantill It's released now. Thank you!