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

Fix loading #86

Merged
merged 6 commits into from Dec 27, 2012

Conversation

Projects
None yet
2 participants
Contributor

candid82 commented Dec 22, 2012

  1. Different loadHandlers don't interfier with each other any more.
  2. Don't trigger load:start on rootObject multiple times.

This PR fixes global loading indicator issue in asda mweb. Thorax.loadHandler is extremely complicated and hard to understand. I am going to send a follow up PR with some refactoring to make it a bit more digestible, but did not want to clutter this PR with other changes.

@candid82 candid82 Fix loading
1. Different loadHandlers don't interfier with each other any more.
2. Don't trigger load:start on rootObject multiple times.
8fb6731
Contributor

candid82 commented Dec 22, 2012

Sigh... fixing unit tests now...

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

src/loading.js
@@ -8,15 +8,16 @@ Thorax.setRootObject = function(obj) {
};
Thorax.loadHandler = function(start, end, context) {
+ var loadInfo;
@kpdecker

kpdecker Dec 22, 2012

Contributor

This is a fundamental change in that instances where this is used on the prototype stack will all share the same loadInfo. The prototype stack is the primary place that we are using this https://github.com/walmartlabs/thorax/blob/master/src/loading.js#L394 meaning that every view instance is going to share the same loading info which seems incorrect to me.

@candid82

candid82 Dec 22, 2012

Contributor

Yeah... I figured. The chalenge was to keep different loadHandlers isolated (if you bind to load:start event several different loadHandlers) and at the same time keep using "self" to track load progress (because of the reason you described). Just committed another attempt to fix this. It does fix global loading indicator issue without breaking unit tests. More unit tests (to cover loadHandlers isolation) are coming.

@kpdecker kpdecker commented on an outdated diff Dec 26, 2012

src/loading.js
}
// If stopping make sure we don't run a start
- clearTimeout(self._loadStart.timeout);
- self._loadStart = undefined;
+ clearTimeout(loadInfo.timeout);
+ loadInfo = self._loadInfo[loadCounter] =undefined;
@kpdecker

kpdecker Dec 26, 2012

Contributor

nit: Space

@kpdecker kpdecker commented on an outdated diff Dec 26, 2012

src/loading.js
Thorax.loadHandler = function(start, end, context) {
+ var loadCounter = _loadCounter++;
@kpdecker

kpdecker Dec 26, 2012

Contributor

nit: Can we use _.uniqueId() so we can remove the _loadCounter var?

Contributor

kpdecker commented Dec 26, 2012

Looks good outside of the two nits.

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

@candid82 candid82 Merge pull request #86 from walmartlabs/loading-fixes
Fix loading
63132e5

@candid82 candid82 merged commit 63132e5 into master Dec 27, 2012

1 check passed

default The Travis build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment