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

Projects
None yet
4 participants
@ghost

ghost commented Oct 20, 2013

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

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Oct 20, 2013

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.)

ghost commented Oct 20, 2013

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

This comment has been minimized.

Show comment
Hide comment
@daffl

daffl Oct 21, 2013

Contributor

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

Contributor

daffl commented Oct 21, 2013

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

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Oct 21, 2013

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)

ghost commented Oct 21, 2013

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

This comment has been minimized.

Show comment
Hide comment
@daffl

daffl Oct 23, 2013

Contributor

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

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

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Oct 25, 2013

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.

Contributor

justinbmeyer commented Oct 25, 2013

@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

This comment has been minimized.

Show comment
Hide comment
@imjoshdean

imjoshdean Oct 29, 2013

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.

Contributor

imjoshdean commented Oct 29, 2013

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

Merge pull request #505 from simpleTechs/patch-1
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

1 check failed

default The Travis CI build failed
Details

@simpleTechs simpleTechs deleted the simpleTechs:patch-1 branch Dec 9, 2013

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