feat(common): add historyGo method to Location service #38890

aahmedayed · 2020-09-17T18:15:14Z

Add new method historyGo, that will let
the user navigate to a specific page from session history identified by its
relative position to the current page.

We add some tests to location_spec.ts to validate the behavior of the
historyGo and forward methods.

Add more tests for location_spec to test location.historyGo(0), location.historyGo(),
location.historyGo(100) and location.historyGo(-100). We also add new tests for
Integration spec to validate the navigation when we using
location#historyGo.

Update the main-es2015 size limits of aio-local-viewengine in
aio-payloads file.

Update the historyGo function docs

Link for the history API documentation for History#go

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

Bugfix
Feature
Code style update (formatting, local variables)
Refactoring (no functional changes, no api changes)
Build related changes
CI related changes
Documentation content changes
angular.io application / infrastructure changes
Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

Yes
No

Other information

atscott

Hi @aahmedayed, thanks for the PR - I think this is a good change overall, just a few items to address.

Add some tests to location_spec.ts to validate the behavior of this new method
Please update the spelling of "methode" and "methodes" in the commit message to be "method" and "methods" and add a link to the history API documentation for History#go.

packages/common/src/location/location.ts

aahmedayed · 2020-09-17T19:16:40Z

Hi @atscott - Thanks for the review, i'll start working on it.

aahmedayed · 2020-09-18T07:26:11Z

Hi @atscott - I've made the adjustments required.

atscott

Great work on updating the mock_platform_location! Just a couple more small requests for testing scenarios so we have sufficient coverage.

packages/common/test/location/location_spec.ts

packages/common/testing/src/mock_platform_location.ts

aahmedayed · 2020-09-18T16:53:16Z

Thank you @atscott, i'm working on it.

aahmedayed · 2020-09-18T19:00:57Z

@atscott - I’ve already finished the adjustments you asked for. ☺️

atscott

LGTM after the fixup of the integration.spec.ts test. Adding approval to allow this to move forward for public API review.

packages/router/test/integration.spec.ts

atscott · 2020-09-18T19:38:39Z

@aahmedayed The CI tests are failing to execute because you may have set up circleci on your fork, but don't have permission to run this level of resource class. Could you please disable CircleCI on your angular fork?

atscott · 2021-03-25T17:24:16Z

public API reviewers, PTAL. Following the discussion in the FW sync today, this change has been made non-breaking by having the new functions be optional.

This opens the door to fixing #13586. In addition, discussions in WICG/navigation-api#32 confirm that the current standard practice for correctly restoring state after a rejected navigation is to use the history.go method.

mary-poppins · 2021-03-25T17:27:37Z

You can preview 81d646a at https://pr38890-81d646a.ngbuilds.io/.

atscott · 2021-03-25T17:55:49Z

presubmit

mary-poppins · 2021-03-25T18:03:58Z

You can preview 9100b03 at https://pr38890-9100b03.ngbuilds.io/.

AndrewKushnir · 2021-03-26T19:33:42Z

@atscott quick question: the historyGo method is marked as optional, but we provide an implementation of this method (that throws an error) in the LocationStrategy abstract class. All classes that extend the base class would not require the historyGo method implementation (so the change is not breaking as you mentioned), but they would have this method (which always throws) available on the class instance. Do we plan to use the historyGo method in the Router code in the followup PR(s) and if so, do we need to somehow differentiate classes that have a "real" implementation of that function vs classes that inherit it from the base class? I might be missing some context here, so I'd like to ask if you could share more info on how/if the historyGo method would be used in followup PR(s). Thank you.

atscott · 2021-03-26T20:35:06Z

@AndrewKushnir Yes, this will be used in the router code in follow-up PRs. The router does not currently do any verification on similar methods that are not implemented in subclasses (or their underlying PlatformLocation) such as ServerPlatformLocation. It wouldn't make much sense to use the router in these contexts. It's not uncommon for the mocks or other implementations to simply throw when one of these functions doesn't make sense for the platform's use-case.

AndrewKushnir · 2021-03-29T23:59:54Z

Thanks for additional information @atscott.

The scenario I was thinking of is:

I have a class that extends abstract LocationStrategy class now and I do not have historyGo method implemented
My class is provided as LocationStrategy (for ex. { provide: LocationStrategy, useClass: MyLocationStrategy }) or using some other token that would make it available in Router code
If my class is then used in the Router and the Router code checks for the historyGo presence (to detect whether historyGo or a fallback method should be called) - it'd be there, but it will just throw and I would need to update implementation of the historyGo method.

I believe the 3rd step might be a breaking change, but it'd be a part of the followup PR, so this PR should be good to go (and the followup PR would be marked as breaking change). Please let me know if my understanding is correct or the above mentioned scenario is not a valid one.

Thank you.

atscott · 2021-03-30T14:01:04Z

@AndrewKushnir - Yes, the change in the router would need to be marked as a breaking change if we made it the default and/or did not make it an opt-in option. It would also be breaking for another reason: even though it would fix currently broken behavior, developers have implemented workarounds for it which would then be incompatible/unnecessary with the history.go fix included. We'll need to evaluate our options at that time.

Add new method `historyGo`, that will let the user navigate to a specific page from session history identified by its relative position to the current page. We add some tests to `location_spec.ts` to validate the behavior of the `historyGo` and `forward` methods. Add more tests for `location_spec` to test `location.historyGo(0)`, `location.historyGo()`, `location.historyGo(100)` and `location.historyGo(-100)`. We also add new tests for `Integration` spec to validate the navigation when we using `location#historyGo`. Update the `historyGo` function docs Note that this was made an optional function in the abstract classes to avoid a breaking change. Because our location classes use `implements PlatformLocation` rather than `extends PlatformLocation`, simply adding a default implementation was not sufficient to make this a non-breaking change. While we could fix the classes internal to Angular, this would still have been a breaking change for any external developers who may have followed our implementations as an example.

mary-poppins · 2021-04-02T17:13:39Z

You can preview dae6f19 at https://pr38890-dae6f19.ngbuilds.io/.

atscott · 2021-04-02T18:05:44Z

presubmit

jelbourn

LGTM

Reviewed-for: public-api, size-tracking

petebacondarwin

Reviewed-for: size-tracking

I am surprised that these fairly small additions add ~700 bytes but probably there was already an increase on master.

angular-automatic-lock-bot · 2021-05-14T16:08:04Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

googlebot added the cla: yes label Sep 17, 2020

pullapprove bot requested review from alxhub and pkozlowski-opensource September 17, 2020 18:15

aahmedayed mentioned this pull request Sep 17, 2020

refactor(router): compute correct history restoration when navigation is cancelled #38884

Closed

14 tasks

atscott added the area: common Issues related to APIs in the @angular/common package label Sep 17, 2020

ngbot bot added this to the needsTriage milestone Sep 17, 2020

atscott added area: router aio: preview target: minor This PR is targeted for the next minor release feature Issue that requests a new feature labels Sep 17, 2020

atscott self-requested a review September 17, 2020 18:30

atscott requested changes Sep 17, 2020

View reviewed changes

packages/common/src/location/location.ts Outdated Show resolved Hide resolved

atscott removed request for pkozlowski-opensource and alxhub September 17, 2020 18:37

pullapprove bot requested review from alxhub and pkozlowski-opensource September 17, 2020 18:37

aahmedayed changed the title ~~feat(common): add goTo methode to Location service~~ feat(common): add goTo method to Location service Sep 17, 2020

aahmedayed requested a review from atscott September 18, 2020 14:37

aahmedayed force-pushed the add-go-and-goto-methodes branch 2 times, most recently from 3ed86c0 to 8e474e7 Compare September 18, 2020 16:13

atscott requested changes Sep 18, 2020

View reviewed changes

packages/common/test/location/location_spec.ts Outdated Show resolved Hide resolved

packages/common/testing/src/mock_platform_location.ts Outdated Show resolved Hide resolved

aahmedayed force-pushed the add-go-and-goto-methodes branch from e110e2a to 365794f Compare September 18, 2020 18:23

aahmedayed requested a review from atscott September 18, 2020 18:24

atscott approved these changes Sep 18, 2020

View reviewed changes

packages/router/test/integration.spec.ts Outdated Show resolved Hide resolved

packages/router/test/integration.spec.ts Outdated Show resolved Hide resolved

packages/router/test/integration.spec.ts Outdated Show resolved Hide resolved

aahmedayed force-pushed the add-go-and-goto-methodes branch from 365794f to 16819c8 Compare September 18, 2020 22:13

atscott force-pushed the add-go-and-goto-methodes branch from 365737b to 81d646a Compare March 25, 2021 17:16

atscott removed the breaking changes label Mar 25, 2021

ngbot bot modified the milestones: needsTriage, Backlog Mar 25, 2021

atscott added target: minor This PR is targeted for the next minor release and removed target: major This PR is targeted for the next major release labels Mar 25, 2021

atscott force-pushed the add-go-and-goto-methodes branch from 81d646a to 9100b03 Compare March 25, 2021 17:55

pullapprove bot requested a review from IgorMinar March 25, 2021 17:55

atscott force-pushed the add-go-and-goto-methodes branch from 9100b03 to dae6f19 Compare April 2, 2021 17:02

jelbourn approved these changes Apr 2, 2021

View reviewed changes

pullapprove bot requested a review from petebacondarwin April 2, 2021 20:09

petebacondarwin approved these changes Apr 2, 2021

View reviewed changes

atscott added the action: merge The PR is ready for merge by the caretaker label Apr 5, 2021

atscott closed this in e05a6f3 Apr 6, 2021

angular-automatic-lock-bot bot locked and limited conversation to collaborators May 14, 2021

aahmedayed deleted the add-go-and-goto-methodes branch June 2, 2021 15:47

aahmedayed restored the add-go-and-goto-methodes branch June 2, 2021 15:47

aahmedayed deleted the add-go-and-goto-methodes branch June 2, 2021 15:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(common): add historyGo method to Location service #38890

feat(common): add historyGo method to Location service #38890

aahmedayed commented Sep 17, 2020 •

edited

atscott left a comment

aahmedayed commented Sep 17, 2020 •

edited

aahmedayed commented Sep 18, 2020 •

edited

atscott left a comment

aahmedayed commented Sep 18, 2020

aahmedayed commented Sep 18, 2020

atscott left a comment

atscott commented Sep 18, 2020

atscott commented Mar 25, 2021

mary-poppins commented Mar 25, 2021

atscott commented Mar 25, 2021

mary-poppins commented Mar 25, 2021

AndrewKushnir commented Mar 26, 2021

atscott commented Mar 26, 2021

AndrewKushnir commented Mar 29, 2021

atscott commented Mar 30, 2021

mary-poppins commented Apr 2, 2021

atscott commented Apr 2, 2021

jelbourn left a comment

petebacondarwin left a comment

angular-automatic-lock-bot bot commented May 14, 2021

feat(common): add historyGo method to Location service #38890

feat(common): add historyGo method to Location service #38890

Conversation

aahmedayed commented Sep 17, 2020 • edited

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

atscott left a comment

Choose a reason for hiding this comment

aahmedayed commented Sep 17, 2020 • edited

aahmedayed commented Sep 18, 2020 • edited

atscott left a comment

Choose a reason for hiding this comment

aahmedayed commented Sep 18, 2020

aahmedayed commented Sep 18, 2020

atscott left a comment

Choose a reason for hiding this comment

atscott commented Sep 18, 2020

atscott commented Mar 25, 2021

mary-poppins commented Mar 25, 2021

atscott commented Mar 25, 2021

mary-poppins commented Mar 25, 2021

AndrewKushnir commented Mar 26, 2021

atscott commented Mar 26, 2021

AndrewKushnir commented Mar 29, 2021

atscott commented Mar 30, 2021

mary-poppins commented Apr 2, 2021

atscott commented Apr 2, 2021

jelbourn left a comment

Choose a reason for hiding this comment

petebacondarwin left a comment

Choose a reason for hiding this comment

angular-automatic-lock-bot bot commented May 14, 2021

aahmedayed commented Sep 17, 2020 •

edited

aahmedayed commented Sep 17, 2020 •

edited

aahmedayed commented Sep 18, 2020 •

edited