search

angular / angular-hint

run-time hinting for AngularJS applications
362 stars 45 forks source link

fix(hint) support multi-page apps #112

Closed gary-b closed 8 years ago

gary-b commented 8 years ago

When multiple pages of a website each had their own angular app Batarang would not load the second app on navigation - the page had to be refreshed. Libraries signal to angular to defer bootstrapping by setting window.name to a string starting with 'NG_DEFER_BOOTSTRAP!'. Code to support protractor was checking if this setting was already present on each load and in response did not set a callback to initiate the bootstrap later on. Hint sets 'NG_DEFER_BOOTSTRAP!' itself however on a page load when protractor isn't present, so when a second page load occurs the code path for protractor is executed incorrectly. Now setting window.name to an Angular Hint specific constant, to avoid conflicts.

I suspect the code to detect protractor is running is wrong as well, looking at https://github.com/angular/protractor/blob/f9b0a92079b55384d4560fef9400bb473672ce9c/lib/protractor.js#L578 it seems to append the current window title onto the end of 'NG_DEFER_BOOTSTRAP!', while the check we do looks for an exact match.

SomeKittens commented 8 years ago

Not sure if that's only Protractor 2.0, or 1.0 as well. @juliemr, mind chiming in on this?

juliemr commented 8 years ago

I'm sorry, @gary-b I can't parse the problem that you think Protractor has here. Can you restate it?

gary-b commented 8 years ago

@juliemr, I'm not suggesting that protractor has a problem. Rather, I think how hint detects protractors presence may be wrong.

It seems that when protractor loads, it sets window.name to 'NG_DEFER_BOOTSTRAP!' + the old window.name. When we try to detect its presence, we check to see if window.name exactly equals 'NG_DEFER_BOOTSTRAP!'. We need to change that code to compare just the start of the window.name string with 'NG_DEFER_BOOTSTRAP!'.

juliemr commented 8 years ago

Ah yes, you're absolutely correct.

SomeKittens commented 8 years ago

(thanks for catching me up).

@gary-b it looks like all this code does is change how we set window.name? Wouldn't we need to do some sort of regex check on line 32?

gary-b commented 8 years ago

@SomeKittens Line 32 will need updated to detect protractor properly. What I have now is sufficient to resolve the original issue. In saying that, I think I might prefer another approach. I'll push an updated commit over the next few days.

gary-b commented 8 years ago

Ok, I changed things about a bit.

You can repro the original issue on this site: http://gary-b.github.io/ng-version-test-apps/index.html Go to the first page, enable angular, then go to the Version 2 link. Notice the page doesn't load. Apply my patch, issue should be resolved. Further details on the new commit message, (if i make any sense at this time of night.)

SomeKittens commented 8 years ago

Thanks for the repo, very tricky bug. Landed in 75a7eec