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

Return a new can.compute every time (new can.view.Scope()).compute() is called #505

Merged
merged 1 commit into from
Oct 29, 2013

Conversation

simpleTechs
Copy link

You need to actually return the new compute, so https://github.com/bitovi/canjs/blob/master/view/bindings/bindings.js#L62 won't get undefined back, resulting in an error here https://github.com/bitovi/canjs/blob/master/view/bindings/bindings.js#L187

@simpleTechs
Copy link
Author

Here's a JSFiddle to show the issue: http://jsfiddle.net/simpleFabian/TWHFs/2/
Comment out the first few lines of js to see the problem.
Also, livebinding will only work on properties that were present when binding was initialized - can this be circumvented somehow? (See in this example only "Middle West"'s checkbox will update correctly, as it has had the property selected before being bound.)

@daffl
Copy link
Contributor

daffl commented Oct 21, 2013

This does seem to make sense but a lot of the tests are failing now.

@simpleTechs
Copy link
Author

Well, yes, many tests are failing, but this is due to the fact I submitted the pull against current head, not against, say, tag 2.0.0. Just tried the latter locally and every single test is fine.
Should I re-submit?

(In fact, 7d38e29 seems to have solved the test-failures)

@daffl
Copy link
Contributor

daffl commented Oct 23, 2013

Travis should be running against the head. I will merge it in locally and see what is happening. Would it be possible to scale the Fiddle down a bit so that I can turn it into an easy test to add for the issue that your change is fixing?

Thanks

@justinbmeyer
Copy link
Contributor

@simpleTechs / @daffl

Anyway you can provide a simple explanation of what's going on? Reading this issue on its own gives no insight to the problem.

@imjoshdean
Copy link
Contributor

The issue is that calling can.view.Scope.prototype.compute doesn't consistently return a compute. It will only return a compute if the requested attribute's parent is observable, otherwise it creates a can.compute and does nothing with it. I'm to assume this boils down to something as simple as "this was probably a typo" or "this was an unintentional slip."

I agree with @daffl, this makes sense. And merging against master it works just fine. My theory as to what cause the test to break was that the pull request came right before cfb6bd0 which fixed some breaking tests.

imjoshdean added a commit that referenced this pull request Oct 29, 2013
Return a new can.compute every time (new can.view.Scope()).compute() is called
@imjoshdean imjoshdean merged commit a01655f into canjs:master Oct 29, 2013
@simpleTechs simpleTechs deleted the patch-1 branch December 9, 2013 23:22
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.

None yet

4 participants