feat(common): add getState method to LocationStrategy interface#45648
feat(common): add getState method to LocationStrategy interface#45648mmaterowski wants to merge 1 commit intoangular:masterfrom
Conversation
|
I'm having trouble understating how my change is failing following test
Seems to work locally when I execute |
atscott
left a comment
There was a problem hiding this comment.
@mmaterowski Thanks for the PR. This does not fully address #34603 because Location still uses PlatformLocation directly rather than going through the new API you're providing here.
In addition, you'll have to note in the commit message that this is a breaking change (add BREAKING CHANGE: <description> to your footer) because it adds a new required class member that any implementors of the LocationStrategy will need to satisfy.
8ae2aaf to
1153241
Compare
Yea, there is known flakiness with a few tests at the moment. Sorry about the friction. |
Adds getState to LocationStrategy interface as it suppose to be the place to control all window.location interactions. BREAKING CHANGE: Adds new required class member that any implementors of the LocationStrategy will need to satisfy. Location does not depend on PlatformLocation anymore.
1153241 to
ef91df6
Compare
|
@atscott thanks for reviewing my PR. |
jessicajaniuk
left a comment
There was a problem hiding this comment.
reviewed-for: public-api
atscott
left a comment
There was a problem hiding this comment.
reviewed-for: public-api
|
Note for team members: Since this is a breaking change, I've triggered global presubmits to identify what needs to be updated. Also adding the blocked label until they necessary internal cleanup has been completed. |
dylhunn
left a comment
There was a problem hiding this comment.
reviewed-for: public-api
|
@atscott by internal cleanup, do you mean TGP breakage? The linked presubmit is not showing any real failures. |
|
@dylhunn The TGP isn't finished (the full train runs don't show up in the UI until they are; only the smoke test is available immediately). From prior work in this area, I know there will be failures that need to be fixed. |
Thanks for clarifying! |
|
I see that you just added the If you want your PR to be merged, it has to pass all the CI checks. If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help. |
|
Internal cleanup is finished. Started a new global presubmit to ensure there weren't any regressions. Then this should be good to go today or tomorrow. |
|
merge assistance: requires internal patch when syncing: http://cl/443689787 |
|
This PR was merged into the repository by commit 31d7c3b. |
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Add getState to LocationStrategy interface as it suppose to be the place to control all window.location interactions
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: #34603
What is the new behavior?
LocationStrategy implements getState() method which will eventually call window.history to access current history state.
Does this PR introduce a breaking change?
Other information
Only classes that implement Abstract
LocationStrategyarePathLocationStrategy(test already present) andHashLocationStrategy. Currently there are no tests forHashLocationStrategy, if you find them necessary for this PR to merge let me know.