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

can.Observe makes can.Deferred into an observable #367

Merged
merged 1 commit into from
May 6, 2013

Conversation

daffl
Copy link
Contributor

@daffl daffl commented May 6, 2013

The current implementation of canMakeObserve is:

canMakeObserve = function( obj ) {
  return obj && (can.isArray(obj) || can.isPlainObject( obj ) || ( obj instanceof can.Observe ));
}

As can.isPlainObject returns true for a can.Deferred, a promise that is part of a model's raw data values will be treated as a plain object to be wrapped as a child can.Observe with observable members itself. This is almost certainly not the intention.

@daffl
Copy link
Contributor

daffl commented Apr 23, 2013

True, that should not be converted. We'll fix it in the next release.

@daffl
Copy link
Contributor

daffl commented May 3, 2013

What actually would be really cool is if can.Observe realizes it is a Deferred, waits for it to complete and then sets the attribute with the result data. Not sure if that always applies though.

@justinbmeyer
Copy link
Contributor

@daffl It shouldn't be converted, but it also shouldn't replace itself, but that's why I want: #366.

daffl added a commit that referenced this pull request May 6, 2013
can.Observe makes can.Deferred into an observable
@daffl daffl merged commit 39d549c into master May 6, 2013
@daffl daffl deleted the deferred-conversion-367 branch May 6, 2013 21:23
@justinbmeyer justinbmeyer restored the deferred-conversion-367 branch May 6, 2013 21:44
@justinbmeyer justinbmeyer deleted the deferred-conversion-367 branch May 6, 2013 21:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants