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

should Observation.add be on the map or the computedProp #149

Closed
justinbmeyer opened this Issue Feb 20, 2017 · 8 comments

Comments

Projects
None yet
4 participants
@justinbmeyer
Copy link
Contributor

justinbmeyer commented Feb 20, 2017

Related: canjs/can-map#30

A consideration is that it might be harder to "trace" the dependency.

@justinbmeyer justinbmeyer added the bug label Apr 19, 2017

@justinbmeyer

This comment has been minimized.

Copy link
Contributor Author

justinbmeyer commented Apr 19, 2017

we should ignore reading the compute, and instead observe directly on the property. This is likely a bit slower, but much more accurate.

@justinbmeyer justinbmeyer added the p2 label Apr 19, 2017

@andrejewski andrejewski self-assigned this Apr 26, 2017

@andrejewski

This comment has been minimized.

Copy link
Contributor

andrejewski commented Apr 26, 2017

@justinbmeyer I wrote some test cases based on canjs/can-map#30 which all passed for can-define. Are you sure this is a problem for DefineMap and not just Map? If it is, I'd like some clarification on the problem here.

@justinbmeyer

This comment has been minimized.

Copy link
Contributor Author

justinbmeyer commented May 23, 2017

Assigning this to @bmomberger-bitovi . @bmomberger-bitovi please close this out while you are working on can-define. The call to Observation.add should be on the map, no the compute (or now observation). This is so we know the map is bound to (things like can-connect's instance store depend on this).

@justinbmeyer

This comment has been minimized.

Copy link
Contributor Author

justinbmeyer commented Jul 31, 2017

DefineMap.extend({
  prop: "string"
  get proped(){  return this.prop }
});

compute(function(){
  return map.proped // should call map.on("proped"), instead -> map._computed.proped.on("change")
})
var c= compute(1);

var Type = DefineMap.extend({
  get thing(){
    return c();
  }
});

var instance = new Type();
var onKeyValue = instance[canSymbol.for("can.onKeyValue")];

instance[canSymbol.for("can.onKeyValue")] = function(){
  ...
}

var c2 = compute(function(){
  return instance.thing;
});

instance._bindEvents._lifecycleCount //=> 0;

@andrejewski andrejewski self-assigned this Aug 3, 2017

@andrejewski

This comment has been minimized.

Copy link
Contributor

andrejewski commented Aug 3, 2017

@justinbmeyer I made the following test case:

QUnit.test('should observe the map key, not value', function (assert) {
	var MyMap = DefineMap.extend({
		foo: 'string',
		get bar () {
			return this.foo;
		}
	});

	var map = new MyMap();
	var newValue = 'baz';
	map.foo = newValue;

	assert.equal(map.bar, newValue, 'map value updated');
});

And another using a compute, for some more indirection I suppose:

QUnit.test('should observe the map key, not value', function (assert) {
	var MyMap = DefineMap.extend({
		foo: 'string',
		get bar () {
			return this.foo;
		}
	});

	var map = new MyMap();
	var newValue = 'baz';
	map.foo = newValue;

	var listeningCompute = compute(function () {
		return map.bar;
	});

	var value = listeningCompute();
	assert.equal(value, newValue, 'map value updated');
});

Both of these cases pass without any code modification.

From what I can tell, the Observation.add call is binding to the key as desired. Perhaps @bmomberger-bitovi did fix this during the reflecting.

@phillipskevin

This comment has been minimized.

Copy link
Contributor

phillipskevin commented Aug 28, 2017

@Sinjhin is going to roll back the changes for this from #244 because it is causing other problems: https://travis-ci.org/canjs/canjs/builds/268510138#L4083-L4098. We will need to to revisit this once we have a way of handling the asynchronous computes created by things like https://github.com/canjs/can-connect/blob/master/can/ref/ref.js#L300.

@phillipskevin

This comment has been minimized.

Copy link
Contributor

phillipskevin commented Sep 1, 2017

canjs/can-observation#109 was also rolled back for the same reason. This will need to be re-done after we have async computes.

@andrejewski andrejewski removed their assignment Oct 6, 2017

@justinbmeyer

This comment has been minimized.

Copy link
Contributor Author

justinbmeyer commented Jan 3, 2018

Closed in 4.0: 1548734

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.