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

Asynchronous virtual properties cause extra recomputes #1915

Closed
dylanrtt opened this Issue Sep 15, 2015 · 3 comments

Comments

Projects
None yet
5 participants
@dylanrtt
Contributor

dylanrtt commented Sep 15, 2015

In the following example, I create an async virtual foo property, and a bar property that depends on it. If you bind to bar, it computes once when foo is undefined, and then recomputes twice after foo is set (3 total times). It should only recompute/log one time after foo is set (2 total times).

var VM = can.Map.extend({
  define: {
    foo: {
      get(lastVal, setVal) {
        console.log('getting foo');
        setTimeout(() => {
          console.log('setting foo');
          setVal(5);
        }, 10);
      }
    },
    bar: {
      get() {
        var foo = this.attr('foo');
        if (foo) {
          console.log(`getting bar... foo = ${foo}`);
          return foo * 2;
        }
      }
    }
  }
});

Using stache binding:
http://jsbin.com/dagozulelu/edit?html,js,console,output

If it matters, in my use case, bar is also an asynchronous virtual property that makes a network request once foo is set.

As a workaround, I am setting foo in my component's inserted event for now.

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Sep 15, 2015

Contributor

Certainly a bug.

Seems like there are 2 problems. First, when setVal is called, it's not dispatching in a batch.

But the more important problem is likely when this.attr('foo') is being called ... it's actually setting up a binding on two things:

  1. the "foo" property, which is correct.
  2. the internal compute of the "foo" property, which is incorrect. This is happening here: https://github.com/bitovi/canjs/blob/minor/map/map.js#L259

The solution is probably to change:

https://github.com/bitovi/canjs/blob/minor/map/map.js#L259

To something like:

can.__notObserve(function(){
  computedAttr.compute();
})

But, I would avoid re-creating the function each time and have a helper like:

var readButDontObserveCompute = can.__notObserve(function(compute){
  return compute()
})

And then call:

return readButDontObserveCompute(computedAttr.compute);
Contributor

justinbmeyer commented Sep 15, 2015

Certainly a bug.

Seems like there are 2 problems. First, when setVal is called, it's not dispatching in a batch.

But the more important problem is likely when this.attr('foo') is being called ... it's actually setting up a binding on two things:

  1. the "foo" property, which is correct.
  2. the internal compute of the "foo" property, which is incorrect. This is happening here: https://github.com/bitovi/canjs/blob/minor/map/map.js#L259

The solution is probably to change:

https://github.com/bitovi/canjs/blob/minor/map/map.js#L259

To something like:

can.__notObserve(function(){
  computedAttr.compute();
})

But, I would avoid re-creating the function each time and have a helper like:

var readButDontObserveCompute = can.__notObserve(function(compute){
  return compute()
})

And then call:

return readButDontObserveCompute(computedAttr.compute);
@whitecolor

This comment has been minimized.

Show comment
Hide comment
@whitecolor

whitecolor Dec 26, 2015

Contributor

I experience I think a like problems with async define.get, I tried solution above, it doesn't seem to work.

Contributor

whitecolor commented Dec 26, 2015

I experience I think a like problems with async define.get, I tried solution above, it doesn't seem to work.

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Dec 26, 2015

Contributor

If you can submit a test, I'll get this in 2.3.8 asap.

Sent from my iPhone

On Dec 26, 2015, at 9:20 AM, Alex notifications@github.com wrote:

I experience I think a like problems with computes, I tried solution above, it doesn't seem to work.


Reply to this email directly or view it on GitHub.

Contributor

justinbmeyer commented Dec 26, 2015

If you can submit a test, I'll get this in 2.3.8 asap.

Sent from my iPhone

On Dec 26, 2015, at 9:20 AM, Alex notifications@github.com wrote:

I experience I think a like problems with computes, I tried solution above, it doesn't seem to work.


Reply to this email directly or view it on GitHub.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment