This repository was archived by the owner on Apr 12, 2024. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 27.4k
fix($snifferProvider): allow history for nwjs applications #15633
Closed
Closed
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
70c2e3a
fix($snifferProvider): allow history for nwjs applications
frederikprijck b874d7e
fixup! fix($snifferProvider): allow history for nwjs applications
frederikprijck 0bf1d8a
fixup! fix($snifferProvider): allow history for nwjs applications
frederikprijck 4a750d1
fixup! fix($snifferProvider): allow history for nwjs applications
frederikprijck File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 theChrome Packaged App
test.In other words, we are not doing anything special if it is
NW
. We have just refined ourChrome Packaged App
-detection check.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.