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

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

Merged
merged 3 commits into from
Sep 2, 2014
Merged

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

merged 3 commits into from
Sep 2, 2014

Conversation

taz
Copy link
Contributor

@taz taz commented Jul 28, 2014

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
Copy link
Owner

ocombe commented Jul 28, 2014

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

@taz
Copy link
Contributor Author

taz commented Jul 28, 2014

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
Copy link
Owner

ocombe commented Jul 28, 2014

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

@taz
Copy link
Contributor Author

taz commented Jul 28, 2014

Exactly. It's being mighty inconsiderate.

@taz
Copy link
Contributor Author

taz commented Jul 28, 2014

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
Copy link
Owner

ocombe commented Jul 28, 2014

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
Copy link
Contributor Author

taz commented Jul 28, 2014

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
Copy link
Contributor Author

taz commented Jul 28, 2014

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
Copy link
Contributor Author

taz commented Jul 28, 2014

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
Copy link
Contributor Author

taz commented Jul 29, 2014

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
Copy link
Contributor Author

taz commented Jul 30, 2014

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
Copy link
Owner

ocombe commented Sep 2, 2014

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