fix($snifferProvider): allow history for nwjs applications #15633
Conversation
| @@ -29,7 +29,8 @@ function $SnifferProvider() { | |||
| $window.chrome && | |||
| ($window.chrome.app && $window.chrome.app.runtime || | |||
| !$window.chrome.app && $window.chrome.runtime && $window.chrome.runtime.id), | |||
| hasHistoryPushState = !isChromePackagedApp && $window.history && $window.history.pushState, | |||
| isNw = $window.nw && $window.nw.process, | |||
There was a problem hiding this comment.
Since isNw is actually part of detecting if it isChromePackagedApp, it is clearer to incorporate it in there:
isNw = ...;
isChromePackagedApp = !isNw && ...;| @@ -25,6 +25,25 @@ describe('$sniffer', function() { | |||
| }); | |||
|
|
|||
|
|
|||
| it('should be true if running inside nw.js', function() { | |||
There was a problem hiding this comment.
I would move this test below should be false on Chrome Packaged Apps (or even incorporate it into that one).
There was a problem hiding this comment.
I put it there to make sure both tests verifying when history should be true are together. But I can move it a bit lower. However, I wouldn't incorporate it into the same test that verifies history being false.
There was a problem hiding this comment.
According to https://github.com/angular/angular.js/pull/15633/files#r97973838, isNw is just a subcase of detecting whether something is a Chrome Packaged App. In that sense, it makes sense to have it incorporated into the Chrome Packaged App test.
In other words, we are not doing anything special if it is NW. We have just refined our Chrome Packaged App-detection check.
There was a problem hiding this comment.
As the intend of this PR was to allow history in NW apps, I'd say it's a good idea to have a test that reflects that change and test it (despite the actual technical implementation).
Incorporate this into the test for Chromed Packaged App sounds odd to me, as NW apps are not actual Chromed Packaged Apps (correct me if I'm wrong here).
Apart from that, I'll be happy to change the test and try to incorporate it into the one you're talking about.
There was a problem hiding this comment.
It was just a suggestion - what makes more sense to me. But I don't feel too strongly about it, so feel free to do it the way it makes more sense to you.
As the intend of this PR was to allow history in NW apps
I see it more like restoring the previous behavior of not blocking history on NW apps, which was incorrectly changed while trying to disable it for Chrome PAckaged Apps.
But like I said, totally up to you. As long as the test is there (and is testing the correct thing), it doesn't have t be incorporated into the other one.
|
BTW, can you change the commit message to not have |
Previously`sniffer` incorrectly detected NW.js applications as `chromePackagedApp` disallowing them to make use of the history API. This commit does detect NW.js applications allowing them to make use of the History API. Fixes #15474
|
I've changed the commit message. I wasn't aware of the issue with using |
Previously `$sniffer` incorrectly detected NW.js apps as Chrome Packaged Apps, disallowing them to use the history API. This commit correctly detects NW.js apps and allows them to use the History API. Fixes angular#15474 Closes angular#15633
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Bug fix
What is the current behavior? (You can also link to an open issue here)
snifferincorrectly detects NW.js applications aschromePackagedAppdisallowing them to make use of the history API.What is the new behavior (if this is a feature change)?
Correctly detecting NW.js applications allowing them to make use of the History API.
Does this PR introduce a breaking change?
No
Please check if the PR fulfills these requirements
Other information:
#15474