feat(common): add historyGo method to Location service by aahmedayed · Pull Request #38890 · angular/angular

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.

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.

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.

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?

mary-poppins · 2020-10-02T20:48:58Z

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

IgorMinar

Hi there, thanks for the PR.

I discussed this with @atscott a bit and I raised two things:

this looks like a speculative API that might or might not end up being used in router to fix #13586 - I'd prefer if we waited for us to solidify that fix and go forward with this API change only if its needed for that fix. The primary reason for this is that the api is not generally useful for applications. And in the case that our router doesn't end up using it, it would be just an extra clutter and additional bytes we need to download to the client within the critical path.
could you consider extending the existing back() and forward() apis by allowing them to accept a number parameter e.g. back(steps: number)/forward(steps: number). The valid parameter value could be a positive integer, or we could support any integer as long as it's within bounds of the current navigation stack (negative values would cause back method to behave as if it were the forward call.

I don't feel strongly about my api name change proposal, but whenever possible I prefer extending existing APIs rather than creating new ones.

aahmedayed · 2020-10-04T23:59:23Z

Thank you @IgorMinar for the review, yes i think we'd rather wait to solidify that fix.
As for your second point, it would probably work, but i'm afraid that the PlatformLocation api's methods would not match the History api's methods.

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

5 of 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 Label used to distinguish feature request from other issues labels Sep 17, 2020

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

atscott requested changes Sep 17, 2020

View reviewed changes

Comment thread packages/common/src/location/location.ts Outdated

atscott removed request for alxhub and pkozlowski-opensource 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

Comment thread packages/common/test/location/location_spec.ts Outdated

Comment thread packages/common/testing/src/mock_platform_location.ts Outdated

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

Comment thread packages/router/test/integration.spec.ts Outdated

Comment thread packages/router/test/integration.spec.ts Outdated

Comment thread packages/router/test/integration.spec.ts Outdated

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

IgorMinar reviewed Oct 2, 2020

View reviewed changes

Comment thread packages/common/testing/src/location_mock.ts Outdated

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

Conversation

aahmedayed commented Sep 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

Uh oh!

atscott left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aahmedayed commented Sep 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aahmedayed commented Sep 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

atscott left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

aahmedayed commented Sep 18, 2020

Uh oh!

aahmedayed commented Sep 18, 2020

Uh oh!

atscott left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

atscott commented Sep 18, 2020

Uh oh!

mary-poppins commented Oct 2, 2020

Uh oh!

IgorMinar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aahmedayed commented Oct 4, 2020

Uh oh!

atscott commented Mar 25, 2021

Uh oh!

mary-poppins commented Mar 25, 2021

Uh oh!

atscott commented Mar 25, 2021

Uh oh!

mary-poppins commented Mar 25, 2021

Uh oh!

AndrewKushnir commented Mar 26, 2021

Uh oh!

atscott commented Mar 26, 2021

Uh oh!

AndrewKushnir commented Mar 29, 2021

Uh oh!

atscott commented Mar 30, 2021

Uh oh!

mary-poppins commented Apr 2, 2021

Uh oh!

atscott commented Apr 2, 2021

Uh oh!

jelbourn left a comment

Choose a reason for hiding this comment

Uh oh!

petebacondarwin left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

aahmedayed commented Sep 17, 2020 •

edited

Loading

aahmedayed commented Sep 17, 2020 •

edited

Loading

aahmedayed commented Sep 18, 2020 •

edited

Loading