Skip to content

Fixes for two issues found using nested views in ui-router#44

Merged
ocombe merged 3 commits into
ocombe:masterfrom
taz:master
Sep 2, 2014
Merged

Fixes for two issues found using nested views in ui-router#44
ocombe merged 3 commits into
ocombe:masterfrom
taz:master

Conversation

@taz

@taz taz commented Jul 28, 2014

Copy link
Copy Markdown
Contributor

Fixed two issues encountered when using ui-router in a specific way.

When a 'root' or parent view is defined and lazy loads modules which are also lazy loaded into a child view, the loader enters a state where it believes files have completed loading before they have.

The 'module redefinition diff' code I added in a previous PR makes this issue worse by not returning a dependency list for modules that it thinks have already loaded.

eg:

In the example below, the 'NavigationView' would be present on every page and feature a login button. Navigation.View.js contains a lazyload module block which instructs it to load User.js. The state is resolved once the 'Navigation.View -> User' dependency is met.

If the user browses to the address http://site.com/login (which is a child of the root node), the app lazy loads a login form into the @content view. Login.View.js also contains a lazyload module block which instructs it to load User.js. The state is resolved once the 'Login.View -> User' dependency is met.

So, the Navigation.View module and the LoginView module both make a series of identical calls to ocLazyLoad to facilitate loading 'User'.

In this case, the page will RANDOMLY fail to load purely based on load latency. Currently the code can return a 'true' result for the file load, but dependencies may still not have been met. This leads to the failure of the second call running very close behind it.

Sometimes it works fine if everything lines up ok and loads quickly. In the absolute best case, it all works, but it ends up loading the User.js file twice. Oops!

The changes in this PR fix both issues. It's been a very frustrating exercise tracking this issue down!

 // State definition
                $stateProvider
                    .state('root', {
                        url: '/',
                        resolve: {
                            NavigationView: ['$ocLazyLoad', function ($lazy) {
                                return $lazy.load({
                                    name: 'Navigation.View',
                                    files: ['app/Navigation/Navigation.View.js']
                                });
                            }]
                        },
                        views: {
                            'navbar@': {
                                controller: 'Navigation.View',
                                templateUrl: 'app/Navigation/Navigation.tpl.html'
                            },
                            'content@': {
                                templateUrl: 'app/Root/Root.tpl.html'
                            }
                        }
                    })

                    .state('root.login', {
                        url: 'login',
                        resolve: {
                            LoginView: ['$ocLazyLoad', function ($lazy) {
                                return  $lazy.load({
                                    name: 'Login.View',
                                    files: ['app/Login/Login.View.js']
                                });
                            }]
                        },
                        views: {
                            'content@': {
                                controller: 'Login.View',
                                templateUrl: 'app/Login/Login.tpl.html'
                            }
                        }
                    });

So, in summary:

Fix 1: Fixed a mistake in the module-redefinition detection block that was causing a 0 dependency return on modules requested from the lazy loader more than once (ie: from different routes).

Fix 2: Added further safeguards to the fileCache to ensure that files loaded very close to each other (ie: in two views on the same page defined in ui-router) do not double-load. This is accomplished by storing the deferred.promise of the file load process in the fileCache rather than a boolean.

B

…ausing a 0 dependency return on modules requested more than once.

Added further safeguards to the fileCache to ensure that files loaded very close to each other (ie: in two views on the same page defined in ui-router) did not double-load.
@ocombe

ocombe commented Jul 28, 2014

Copy link
Copy Markdown
Owner

Wow that looks like a nice headache ! I'll check it out later today and test it.
Nice new avatar by the way :)

@taz

taz commented Jul 28, 2014

Copy link
Copy Markdown
Contributor Author

Hold that thought, looks like there's still a bug in there somewhere. Instead of failing 1 in 10, it's more like 1 in 40, but it's still failing, just in a different spot.

Thanks re: the new avatar... It is a Pademelon, he decided to wander up and pose for us (very tame), so I took his picture :-)

@ocombe

ocombe commented Jul 28, 2014

Copy link
Copy Markdown
Owner

Damn I hate those hide&seek bugs !
All bugs should be simple and throw a nice red alert in the console :)

@taz

taz commented Jul 28, 2014

Copy link
Copy Markdown
Contributor Author

Exactly. It's being mighty inconsiderate.

@taz

taz commented Jul 28, 2014

Copy link
Copy Markdown
Contributor Author

So... When I add a whole heap of debug output, it stops doing it. I remove the debug output and it throws error about the module being undefined in the try/catch around line 385 every 40 or so loads.

It's like it's not quite done loading the module in then getModule is called. shrug

I'm falling asleep now, I'll pick this up again tomorrow.

@ocombe

ocombe commented Jul 28, 2014

Copy link
Copy Markdown
Owner

Ok, if it may be a question of digest ? Maybe put your code in a $timeout of in a $rootScope.$evalAsync ? (http://www.bennadel.com/blog/2605-scope-evalasync-vs-timeout-in-angularjs.htm)

@taz

taz commented Jul 28, 2014

Copy link
Copy Markdown
Contributor Author

I was thinking that too, but I couldn't work out how to wrap it properly.

I've just read some more on $timeout and how it returns/resolves a promise and I think I've got it.

Just madly refreshing the page now to see if it's fixed...

@taz

taz commented Jul 28, 2014

Copy link
Copy Markdown
Contributor Author

Interesting read re: eval.async. I'll change it over tomorrow and see if it re-breaks it!

In this case, $timeout is needed as it returns a promise. evalAsync doesn't. I'm leveraging that returned promise to pass through the result from the loadDependencies function. :-)

Now, I'm REALLY going to bed. Good night sir!

@taz

taz commented Jul 28, 2014

Copy link
Copy Markdown
Contributor Author

Nope it's still randomly (not often) failing in that try/catch block even with the $timeout in place.

There's something odd happening with lazy loaded modules that are loaded as a dependency from another module that don't have any dependencies of their own to load... but only when requested simultaneously from more than one view. Obscure yes? :-)

…ader too early due to premature entry into the moduleCache. Solution is to ignore the moduleCache for this part of the load.
@taz

taz commented Jul 29, 2014

Copy link
Copy Markdown
Contributor Author

I think it was getting confused due to the moduleCache being updated out-of-order by the other route.

So, to determine if it was allowed to load, it was asking "does the module exist OR is it in the module cache". The exist test returned false, but the moduleCache returned truthy.

On the very next run, this caused loadDep to fail as the module doesn't actually exist. We know about it and it's in the loading process, but it's just not quite ready by the time it runs. In my logging I was almost always seeing the module load juuuuuust after the error occured! So close.

I haven't been able to get it to error out since the change, but it's early days yet. I'll remove all the debug code now and watch it throughout the day while I'm building another component.

@taz

taz commented Jul 30, 2014

Copy link
Copy Markdown
Contributor Author

No errors since the last commit. :-) I think it's all good.

@ocombe ocombe merged commit 4f5f854 into ocombe:master Sep 2, 2014
ocombe added a commit that referenced this pull request Sep 2, 2014
With nested views & parallel lazy loads you could sometimes get a concurrency problem and load files twice.

Closes #44
@ocombe

ocombe commented Sep 2, 2014

Copy link
Copy Markdown
Owner

I should have merged this way sooner, sorry for the delay and thanks for the fix !

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