Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Add unit test for model.load bug #60

Merged
merged 20 commits into from Dec 27, 2012

Conversation

Projects
None yet
4 participants
Contributor

candid82 commented Dec 10, 2012

Sorry for trailing whitespace fixes. I did not do that, Sublime did :)

This is a unit test for #58

I also noticed that in 'data load on route change sends load events' test failback gets called twice, which doesn't seem right.

Owner

eastridge commented Dec 10, 2012

@candid, do you know why the tests are still failing if this provides the fix? Also, would you mind adding to the docs in docs/loading.md?

Owner

eastridge commented Dec 10, 2012

Ah, I see this is just the failing test.

Contributor

candid82 commented Dec 10, 2012

Yeah, I am looking into the fix right now.

Contributor

candid82 commented Dec 11, 2012

Ok, here is my fix. I discussed this with Kevin and we agreed that binding to 'route' event within bindToRoute introduces the problem of "how to reliably unbind when ajax is complete". I could not come up with an acceptable solution. Instead, I removed binding to 'route' event from bindToRoute completely and only call failback when bindToRoute's return function is called. This slightly changes behavior (and I had to fix a few unit tests), but I think it's logical and consistent.

Owner

eastridge commented Dec 11, 2012

Couple of things:

  • what setting in sublime text is affecting the spacing? Some of it looks like good cleanup, other whitespace I have no idea what's going on or why it changed.
  • this is outside my area of expertise. I'm ok with merging (except for one issue below) if @kpdecker is good with the fix.
  • Regarding Thorax.routeCount, is there any reason you see a need for that to be exposed? If not just make it a local function. If it's going to be public can you add docs in docs/loading.md?

Thanks! ~ Ryan

Contributor

candid82 commented Dec 11, 2012

Whitespaces are affected by "trim_trailing_white_space_on_save": true.
No reason for routeCount to be exposed. Made it a local function.

@kpdecker kpdecker commented on the diff Dec 11, 2012

src/loading.js
@@ -309,20 +299,7 @@ _.each(klasses, function(DataClass) {
Thorax.forwardLoadEvents(this, rootObject, true);
}
- var self = this;
- loadData.call(this, callback,
- function(isError) {
- // Route changed, kill it
- if (!isError) {
- if (self._request) {
- self._aborted = true;
- self._request.abort();
@kpdecker

kpdecker Dec 11, 2012

Contributor

This is a problem. This caused the ajax exec and consequently the loading events to end immediately after the route change occurred. This behavior needs to remain the same otherwise there will be cases where the loading indicator display is tied to data that is going to be thrown away.

@candid82

candid82 Dec 11, 2012

Contributor

I'll need to think about this.

@candid82

candid82 Dec 14, 2012

Contributor

OK, here is my attempt to address the issue. Basically I loadData function is now responsible for handling 'route' events (since it actually has hooks to ajax request and can unbind on ajax.complete, unlike bindToRoute).

Contributor

kpdecker commented Dec 11, 2012

The trailing spaces package is nice too as it will color all trailing whitespace in an annoying contrasting color so you know it's there but you still have control over when you fix it so you can keep your diffs clean :)

Contributor

candid82 commented Dec 11, 2012

Thanks, I'll give it a try. Even though my plan was to fix all the code in world with my sublime settings :)

Contributor

kpdecker commented Dec 12, 2012

It's a bit annoying to do thing manually, but having diffs that are 90%
whitespace changes are a bit harder to follow :)

On Tue, Dec 11, 2012 at 3:02 PM, Roman Bataev notifications@github.comwrote:

Thanks, I'll give it a try. Even though my plan was to fix all the code in
world with my sublime settings :)


Reply to this email directly or view it on GitHubhttps://github.com/walmartlabs/thorax/pull/60#issuecomment-11263856.

Contributor

mogstad commented Dec 12, 2012

That's why GitHub has the w=1 option: https://github.com/walmartlabs/thorax/pull/60/files?w=1

On 2012-12-11, at 5:24 PM, Kevin Decker notifications@github.com wrote:

It's a bit annoying to do thing manually, but having diffs that are 90%
whitespace changes are a bit harder to follow :)

On Tue, Dec 11, 2012 at 3:02 PM, Roman Bataev notifications@github.comwrote:

Thanks, I'll give it a try. Even though my plan was to fix all the code in
world with my sublime settings :)


Reply to this email directly or view it on GitHubhttps://github.com/walmartlabs/thorax/pull/60#issuecomment-11263856.


Reply to this email directly or view it on GitHub.

Contributor

kpdecker commented Dec 12, 2012

Yes but you can't comment on lines in ignore white space mode :)

Sent from my mobile device

On Dec 11, 2012, at 9:13 PM, Bjarne Mogstad notifications@github.com
wrote:

That's why GitHub has the w=1 option:
https://github.com/walmartlabs/thorax/pull/60/files?w=1

On 2012-12-11, at 5:24 PM, Kevin Decker notifications@github.com wrote:

It's a bit annoying to do thing manually, but having diffs that are 90%
whitespace changes are a bit harder to follow :)

On Tue, Dec 11, 2012 at 3:02 PM, Roman Bataev notifications@github.comwrote:

Thanks, I'll give it a try. Even though my plan was to fix all the code
in
world with my sublime settings :)


Reply to this email directly or view it on GitHub<
https://github.com/walmartlabs/thorax/pull/60#issuecomment-11263856>.


Reply to this email directly or view it on GitHub.


Reply to this email directly or view it on
GitHubhttps://github.com/walmartlabs/thorax/pull/60#issuecomment-11275076.

@kpdecker kpdecker and 1 other commented on an outdated diff Dec 18, 2012

test/src/test.loading.js
@@ -533,7 +550,7 @@ describe('loading', function() {
fragment = "bar";
Backbone.history.trigger('route');
expect(callback).to.not.have.been.called;
- expect(failback).to.have.been.calledOnce;
+ expect(failback).to.not.have.been.called;
@kpdecker

kpdecker Dec 18, 2012

Contributor

Continuing the thread above, it seems like this is a change in behavior that is incorrect. We want this to call the failback to occur.

@candid82

candid82 Dec 18, 2012

Contributor

Failback will be called only after bindToRoute's returned value is called. bindToRoute doesn't trigger failback immediately after route change anymore. Binding to route change within bindToRoute would leave timely unbinding to the mercy of the client code, which would have to always call bindToRoute's return value (and the client code only wants to call it on ajax success). The code that binds to route change is now in loadData function which can unbind on ajax complete.

@kpdecker

kpdecker Dec 19, 2012

Contributor

We want bindToRoute to fail early as the loading indicator is linked to it. If it does not fail early then the user is stuck in a state where the loading indicator is displayed for something that we know will be hidden will it not?

@candid82

candid82 Dec 19, 2012

Contributor

As far as I can see loading indicator is bound to 'load:start' and 'load:end' events, that are handled by model.fetch method (which is called by model.load). model.load will fail early (immediately after route change) and it doesn't use bindToRoute anymore.

@kpdecker

kpdecker Dec 20, 2012

Contributor

Perhaps I'm missing something here. So the bindToRoute behavior has been moved to load and the actual bindToRoute implementation doesn't notify until later?

@candid82

candid82 Dec 20, 2012

Contributor

Yes. I can change bindToRoute behavior back (since it's not used in load any more, it won't cause any issues there), but I think the old behavior is broken by design. With the old behavior, once you call bindToRoute you cannot cancel it. Failback will be executed on route change even if you don't want it to (like if you do ajax with success: bindToRoute(...) and ajax fails).

@kpdecker

kpdecker Dec 21, 2012

Contributor

Ok I think the goal of this pull request just clicked. Sorry for being dense.

I think rather than implementing two different paths that effectively implement bind to route we should implement a cleanup option for bind to route to make it cancelable and then the ajax failure handler can call that.

What about something like:

var bound = bindToRoute(....);

this.fetch({ success: bound, failure: function() { bound.cancel(); /* other stuff */ });

Alternatively since it looks like we have changed the path to bindToRoute we can also change the return without as much concern so bind to route could return a hash for the success and error handler rather than trying to bolt it on the callback to maintain API compatibility. (The Thorax.Router.bindToRoute version probably should retain the old return format...)

@candid82

candid82 Dec 24, 2012

Contributor

Added cancel() method to bindToRoute return value and made bindToRoute fail early as it did before.

@kpdecker kpdecker and 1 other commented on an outdated diff Dec 18, 2012

test/src/test.loading.js
expect(success).to.not.have.been.called;
- expect(failback).to.have.been.calledTwice;
@kpdecker

kpdecker Dec 18, 2012

Contributor

Why was it calling this twice before but only once now?

@candid82

candid82 Dec 18, 2012

Contributor

I also moved this line above .respond. It will be called second time after .respond, which I think is actually sinon's issue - the request has been aborted by the time we call .respond, so it should not probably do anything. With real ajax requests the failback will only be called once - immediately after trigger('route').

@kpdecker kpdecker and 1 other commented on an outdated diff Dec 18, 2012

test/src/test.loading.js
func = reset();
Backbone.history.trigger('route');
func();
- expect(callback).to.have.been.calledOnce;
- expect(failback).to.not.have.been.called;
+ expect(callback).to.not.have.been.called;
+ expect(failback).to.have.been.calledOnce;
@kpdecker

kpdecker Dec 18, 2012

Contributor

Looking at the comment above changing this behavior looks like it will break the backbone implementation. This test is there to ensure that we don't break under backbone implementations that call the route event before the execution of the route callback.

@candid82

candid82 Dec 18, 2012

Contributor

bindToRoute doesn't depend on the timing of route event anymore. It will only call callback of failback after its returned value is called. Failback will be called if at least one route event happened in between.

@kpdecker kpdecker and 1 other commented on an outdated diff Dec 26, 2012

src/loading.js
return;
}
+ routeChanged = true;
+ Backbone.history.off('route', routeHandler);
@kpdecker

kpdecker Dec 26, 2012

Contributor

nit: should this call res.cancel to keep the logic the same?

@candid82

candid82 Dec 27, 2012

Contributor

Good point. Done.

Contributor

kpdecker commented Dec 26, 2012

This looks fine outside of the nit comment. Thanks for following up on this!

@candid82 candid82 added a commit that referenced this pull request Dec 27, 2012

@candid82 candid82 Merge pull request #60 from walmartlabs/model-load-bug-test
Add unit test for model.load bug
932ed3f

@candid82 candid82 merged commit 932ed3f into master Dec 27, 2012

1 check passed

default The Travis build passed
Details

@candid82 candid82 deleted the model-load-bug-test branch Dec 27, 2012

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