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

Add pushToStack() and popFromStack() methods to allow async Observations #52

Closed
wants to merge 2 commits into from

Conversation

BigAB
Copy link
Contributor

@BigAB BigAB commented Feb 6, 2017

Resolves #51

When there is no func value passed to an Observation, the methods
pushToStack() and popFromStack() can be used to make observations outside
of the context of a function call.

I should add, as a bit of a constraint, Justin suggested I should avoid changing the code in start() or update(), because they are optimized and I wouldn't want to wreck performance that is already in place to add a new feature.

When there is no `func` value passed to an Observation, the methods
pushToStack() and popFromStack() can be used to make observations outside
of the context of a function call.
* Starts observing all observables in general until `.popFromStack()` is called.
*
*/
pushToStack() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use ES5.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops, sorry, force of habit

@@ -101,6 +101,11 @@ function Observation(func, context, compute){
this.childDepths = {};
this.ignore = 0;
this.needsUpdate= false;
if (!func) {
this.start = function(){
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is start needed?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this function need to be re-generated? Could only one be created and just use a variable?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

start is needed because it is called in update(), and if I modified update to not call start(), I wasn't getting updates. There may be a better way to do this though, I am open to suggestion.

I'll declare the function outside of the constructor and reuse it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An alternative to inserting the start method there, could be modifying update() to be:

	update: function(batchNum){
		if(this.needsUpdate) {
			remaining.updates--;
		}
		this.needsUpdate = false;
		if(this.bound) {
			// Keep the old value.
			var oldValue = this.value;
			this.oldValue = null;
			// Get the new value and register this event handler to any new observables.
			if (this.func) {
				this.start();
			}
			if(oldValue !== this.value || !this.func) {
				this.compute.updater(this.value, oldValue, batchNum);
				return true;
			}
		}
	},

Would that be a more preferable approach?

@BigAB
Copy link
Contributor Author

BigAB commented Feb 6, 2017

This is repeated but I wanted it out where it could be seen

An alternative to inserting the start method there, could be modifying update() to be:

	update: function(batchNum){
		if(this.needsUpdate) {
			remaining.updates--;
		}
		this.needsUpdate = false;
		if(this.bound) {
			// Keep the old value.
			var oldValue = this.value;
			this.oldValue = null;
			// Get the new value and register this event handler to any new observables.
			if (this.func) {
				this.start();
			}
			if(oldValue !== this.value || !this.func) {
				this.compute.updater(this.value, oldValue, batchNum);
				return true;
			}
		}
	},

Would that be a more preferable approach?

@BigAB
Copy link
Contributor Author

BigAB commented Feb 8, 2017

We decided on a call that this was not the best way to expose a way to resolve the issue.

Rather than this way, I'll create a new PR just exposing some of the lower level stateful parts of can-observation, so that one could subclass (or whatever) in their application code

@BigAB BigAB closed this Feb 8, 2017
@BigAB BigAB deleted the observe-reads-outside-of-a-function branch February 8, 2017 16:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants