can.Map constructor is observable #2220

Closed
dylanrtt opened this Issue Jan 29, 2016 · 5 comments

Comments

Projects
None yet
4 participants
@dylanrtt
Contributor

dylanrtt commented Jan 29, 2016

If I define a compute that returns a new can.Map(), changes to the map cause the compute to re-execute.

http://jsbin.com/suxayikepu/edit?js,console

var c = can.compute(function() {
  return new can.Map();
});

c.bind('change', function(){});

var map = c();

// recomputes c
map.attr('foo', 'bar');

Shouldn't the compute in this example only ever be evaluated once? It appears to be binding to its own return value.

@dylanrtt dylanrtt changed the title from Computes re-evaluate when their return values change to Computes bind to their return values Jan 29, 2016

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Jan 29, 2016

Contributor

Yes. Seems like a bug.

Sent from my iPhone

On Jan 28, 2016, at 6:00 PM, dylanrtt notifications@github.com wrote:

If I define a compute that returns a new canMap(), changes to the map cause the compute to re-execute

http://jsbincom/suxayikepu/edit?html,js,console,output

var c = cancompute(function() {
return new canMap();
});

cbind('change', function(){});

var map = c();

// recomputes c
mapattr('foo', 'bar');
Shouldn't the compute in this example only ever be evaluated once? It appears to be binding to its own return value


Reply to this email directly or view it on GitHub.

Contributor

justinbmeyer commented Jan 29, 2016

Yes. Seems like a bug.

Sent from my iPhone

On Jan 28, 2016, at 6:00 PM, dylanrtt notifications@github.com wrote:

If I define a compute that returns a new canMap(), changes to the map cause the compute to re-execute

http://jsbincom/suxayikepu/edit?html,js,console,output

var c = cancompute(function() {
return new canMap();
});

cbind('change', function(){});

var map = c();

// recomputes c
mapattr('foo', 'bar');
Shouldn't the compute in this example only ever be evaluated once? It appears to be binding to its own return value


Reply to this email directly or view it on GitHub.

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Jan 29, 2016

Contributor

my best is there is a call to call can.Map.keys somewhere during init that shouldn't be there.

Contributor

justinbmeyer commented Jan 29, 2016

my best is there is a call to call can.Map.keys somewhere during init that shouldn't be there.

@dylanrtt

This comment has been minimized.

Show comment
Hide comment
@dylanrtt

dylanrtt Jan 29, 2016

Contributor

Yeah, this doesn't happen with just a compute, so it's a can.Map init issue.

I can do this as a workaround:

var c = can.compute(function() {
  var map;
  can.__notObserve(function() {
    map = new can.Map();
  })();
  return map;
});

Tracing was pretty easy. During setup, it sets the data (an empty object in this case) and that eventually leads to can.each which calls keys(). I don't know enough about the internals of the framework to know whether adding __notObserve somewhere in the chain would be the best solution as opposed to something else.

Contributor

dylanrtt commented Jan 29, 2016

Yeah, this doesn't happen with just a compute, so it's a can.Map init issue.

I can do this as a workaround:

var c = can.compute(function() {
  var map;
  can.__notObserve(function() {
    map = new can.Map();
  })();
  return map;
});

Tracing was pretty easy. During setup, it sets the data (an empty object in this case) and that eventually leads to can.each which calls keys(). I don't know enough about the internals of the framework to know whether adding __notObserve somewhere in the chain would be the best solution as opposed to something else.

@dylanrtt dylanrtt changed the title from Computes bind to their return values to new can.Map() is observable Jan 29, 2016

@dylanrtt dylanrtt changed the title from new can.Map() is observable to can.Map constructor is observable Jan 29, 2016

@rjgotten

This comment has been minimized.

Show comment
Hide comment
@rjgotten

rjgotten Jan 29, 2016

@dylanrtt

During setup, it sets the data (an empty object in this case) and that eventually leads to can.each which calls keys(). I don't know enough about the internals of the framework to know whether adding __notObserve somewhere in the chain would be the best solution as opposed to something else.

The problem is that the setup method calls into Map.prototype.attr with an object-based setter. This takes the code-path to Map.prototype._setAttrs, which uses the observable Map.prototype.each instead of the non-observable Map.prototype._each when iterating over its current values.

I'd say this is a general bug with tripping an observer hook during the map.attr({ ... }) method signature that just so happens to also surface as part of the constructor.

(Setters are never supposed to be observed, right? Just getters...)

@dylanrtt

During setup, it sets the data (an empty object in this case) and that eventually leads to can.each which calls keys(). I don't know enough about the internals of the framework to know whether adding __notObserve somewhere in the chain would be the best solution as opposed to something else.

The problem is that the setup method calls into Map.prototype.attr with an object-based setter. This takes the code-path to Map.prototype._setAttrs, which uses the observable Map.prototype.each instead of the non-observable Map.prototype._each when iterating over its current values.

I'd say this is a general bug with tripping an observer hook during the map.attr({ ... }) method signature that just so happens to also surface as part of the constructor.

(Setters are never supposed to be observed, right? Just getters...)

@justinbmeyer justinbmeyer self-assigned this Feb 3, 2016

@justinbmeyer justinbmeyer added this to the 2.3.14 milestone Feb 3, 2016

@justinbmeyer justinbmeyer added the bug label Feb 4, 2016

@daffl daffl modified the milestones: 2.3.15, 2.3.14 Feb 5, 2016

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Feb 13, 2016

Contributor

@rjgotten Yeah, setters shouldn't be observable. Going to work on a fix now. Thanks!

Contributor

justinbmeyer commented Feb 13, 2016

@rjgotten Yeah, setters shouldn't be observable. Going to work on a fix now. Thanks!

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