Navigation Menu

Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(router): remove preserveQueryParams symbol #38762

Closed
wants to merge 4 commits into from
Closed

fix(router): remove preserveQueryParams symbol #38762

wants to merge 4 commits into from

Conversation

josephperrott
Copy link
Member

Remove preserveQueryParams as it was deprecated for removal in v4, use
queryParamsHandling="preserve" instead.

@josephperrott josephperrott force-pushed the remove-preserve-query-params branch 2 times, most recently from ec0e0f5 to 8ac7f8f Compare September 10, 2020 18:21
@josephperrott josephperrott added target: major This PR is targeted for the next major release action: review The PR is still awaiting reviews from at least one requested reviewer area: router labels Sep 11, 2020
@ngbot ngbot bot modified the milestone: needsTriage Sep 11, 2020
@josephperrott josephperrott marked this pull request as ready for review September 15, 2020 15:50
atscott
atscott approved these changes Sep 15, 2020
View reviewed changes
Copy link
Contributor

@atscott atscott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but would add blocked label to wait for the migration PR to merge.

IgorMinar
IgorMinar approved these changes Sep 17, 2020
View reviewed changes
Copy link
Contributor

@IgorMinar IgorMinar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one has been deprecated for a long time, so it's great that we are removing it!

Do we understand the impact of this change? it's likely low, but we should verify. I see at least two instances in google3, I haven't tried to search github.

If a migration was part of this PR then this change would be totally fine, but I get that creating migrations for absolutely everything doesn't always makes sense const/benefits-wise. Maybe get @mgechev to weigh in on skipping a migration?

If we are not going to create a migration for this change, then we should at least document the decision in the PR with a brief justification, something along the lines: "This api has been deprecated for a long time, and we don't have evidence that it's being used.

And additionally we should highlight this change in the v11 release notes / update guide.

Otherwise lgtm.

aio/content/guide/deprecations.md
@@ -117,13 +116,6 @@ Tip: In the [API reference section](api) of this doc site, deprecated APIs are i
| --- | ----------- | --------------------- | ----- |
| [`ngModel` with reactive forms](#ngmodel-reactive) | [`FormControlDirective`](api/forms/FormControlDirective) | v6 | none |

{@a router}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of deleting this section, can you please move it to the removed apis section: https://angular.io/guide/deprecations#removed-apis-1

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please update the deprecations.md as mentioned above. thanks!

@IgorMinar IgorMinar added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Sep 17, 2020
petebacondarwin
petebacondarwin approved these changes Sep 18, 2020
View reviewed changes
Copy link
Member

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed-for: public-api

LGTM after @IgorMinar's comments are addressed.

AndrewKushnir
AndrewKushnir approved these changes Sep 29, 2020
View reviewed changes
Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed-for: public-api

@pullapprove pullapprove bot requested a review from alxhub September 29, 2020 00:04
IgorMinar
IgorMinar suggested changes Oct 5, 2020
View reviewed changes
Copy link
Contributor

@IgorMinar IgorMinar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just resubmitting #38762 (review) to get this out of my review queue for now

Reviewed-for: public-api, docs-packaging-and-releasing

@josephperrott josephperrott force-pushed the remove-preserve-query-params branch 4 times, most recently from 5f96a67 to d00a610 Compare October 12, 2020 19:58
IgorMinar
IgorMinar suggested changes Oct 12, 2020
View reviewed changes
Copy link
Contributor

@IgorMinar IgorMinar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just a few nits, the rest looks good.

packages/core/schematics/test/preserve_query_params_migration_spec.ts Outdated Show resolved Hide resolved
packages/core/schematics/test/preserve_query_params_migration_spec.ts Outdated Show resolved Hide resolved
packages/core/schematics/test/preserve_query_params_migration_spec.ts Outdated Show resolved Hide resolved
packages/core/schematics/test/preserve_query_params_migration_spec.ts Outdated Show resolved Hide resolved
packages/core/schematics/migrations/router-preserve-query-params/README.md Outdated Show resolved Hide resolved
aio/content/guide/deprecations.md
@@ -117,13 +116,6 @@ Tip: In the [API reference section](api) of this doc site, deprecated APIs are i
| --- | ----------- | --------------------- | ----- |
| [`ngModel` with reactive forms](#ngmodel-reactive) | [`FormControlDirective`](api/forms/FormControlDirective) | v6 | none |

{@a router}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please update the deprecations.md as mentioned above. thanks!

@josephperrott
fix(router): remove preserveQueryParams symbol
d88d120 
Remove preserveQueryParams as it was deprecated for removal in v4, use
queryParamsHandling="preserve" instead.

BREAKING CHANGE: preserveQueryParams has been removed, use
queryParamsHandling="preserve" instead
@josephperrott
fix(router): create schematic for preserveQueryParams
fee15a7 
Create a schematic for migrating preserveQueryParams to use queryParamsHandler
instead.
@josephperrott
fixup! fix(router): create schematic for preserveQueryParams
a497bd0
@josephperrott
docs: update removed section in deprecations to reference v11
d19677b 
Updates the removed section in the deprecations guide to talk about APIs removed
in v11 rather than the pervious versions.
IgorMinar
IgorMinar approved these changes Oct 14, 2020
View reviewed changes
Copy link
Contributor

@IgorMinar IgorMinar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks!

Reviewed-for: public-api

IgorMinar
IgorMinar approved these changes Oct 14, 2020
View reviewed changes
Copy link
Contributor

@IgorMinar IgorMinar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed-for: global-approvers, docs-packaging-and-releasing, dev-infra,

@IgorMinar IgorMinar added action: merge The PR is ready for merge by the caretaker and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews action: review The PR is still awaiting reviews from at least one requested reviewer labels Oct 14, 2020
@atscott atscott closed this in 783a5bd Oct 14, 2020
atscott pushed a commit that referenced this pull request Oct 14, 2020
@josephperrott @atscott
fix(router): create schematic for preserveQueryParams (#38762)
93ee05d 
Create a schematic for migrating preserveQueryParams to use queryParamsHandler
instead.

PR Close #38762
atscott pushed a commit that referenced this pull request Oct 14, 2020
@josephperrott @atscott
docs: update removed section in deprecations to reference v11 (#38762)
b0a8b69 
Updates the removed section in the deprecations guide to talk about APIs removed
in v11 rather than the pervious versions.

PR Close #38762
@josephperrott josephperrott deleted the remove-preserve-query-params branch October 14, 2020 17:41
@angular-automatic-lock-bot
Copy link

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Nov 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@petebacondarwin petebacondarwin petebacondarwin approved these changes

@IgorMinar IgorMinar IgorMinar approved these changes

@atscott atscott atscott approved these changes

@AndrewKushnir AndrewKushnir AndrewKushnir approved these changes

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker area: router cla: yes target: major This PR is targeted for the next major release
Projects
None yet
Milestone
needsTriage
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@josephperrott @petebacondarwin @IgorMinar @atscott @AndrewKushnir @googlebot