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.

Sign up for GitHub

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

[FEATURE routing-router-service-refresh] Add a refresh method to the router service #19471

Merged
merged 1 commit into from
May 24, 2021
Merged

[FEATURE routing-router-service-refresh] Add a refresh method to the router service #19471

merged 1 commit into from
May 24, 2021

Conversation

Windvis
Copy link
Contributor

@Windvis Windvis commented Mar 19, 2021

This adds the refresh method to the RouterService as described in RFC 631.

RFC PR: emberjs/rfcs#631

@Windvis
Copy link
Contributor Author

Windvis commented Mar 19, 2021 •
edited
Loading

The RFC comments mentioned that the Route#refresh method should be reimplemented to use the RouterService#refresh method. Should that be done? And if so, should it be done as part of this PR? I'll do it in a follow-up PR.

@Windvis Windvis force-pushed the rfc-631/router-service-refresh branch from bdc223c to 29613d3 Compare March 19, 2021 21:54
@Windvis Windvis force-pushed the rfc-631/router-service-refresh branch 2 times, most recently from ff5a751 to ef18ee2 Compare March 19, 2021 22:32
@Windvis Windvis mentioned this pull request Mar 20, 2021
Closed
@Windvis Windvis marked this pull request as draft March 23, 2021 11:40
@Windvis Windvis marked this pull request as ready for review March 23, 2021 21:14
@Windvis Windvis force-pushed the rfc-631/router-service-refresh branch from 3995cb6 to 8303b9c Compare March 23, 2021 21:49
@Windvis Windvis changed the title [FEATURE RFC 631] Add a refresh method to the router service [FEATURE routing-router-service-refresh] Add a refresh method to the router service Mar 23, 2021
@rwjblue
Copy link
Member

rwjblue commented Mar 24, 2021

Mind rebasing (there is a conflict now in packages/@ember/canary-features/index.ts)?

rwjblue
rwjblue requested changes Mar 24, 2021
View reviewed changes
Copy link
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

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

Thanks for working on this!!

packages/@ember/-internals/routing/lib/services/router.ts Outdated Show resolved Hide resolved
packages/@ember/-internals/routing/lib/services/router.ts Outdated Show resolved Hide resolved
packages/ember/tests/routing/router_service_test/refresh_test.js Outdated Show resolved Hide resolved
@Windvis Windvis force-pushed the rfc-631/router-service-refresh branch from 8303b9c to 34775c3 Compare March 24, 2021 19:04
@Windvis
[FEATURE routing-router-service-refresh] Add a refresh method to the …
bff7766 
…router service

This adds the refresh method to the RouterService as described in RFC 631.

RFC PR: emberjs/rfcs#631
@Windvis Windvis force-pushed the rfc-631/router-service-refresh branch from 34775c3 to bff7766 Compare March 25, 2021 18:59
@Windvis Windvis requested a review from rwjblue March 25, 2021 19:07
rwjblue
rwjblue approved these changes May 24, 2021
View reviewed changes
Copy link
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

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

Sorry for the delay here @Windvis, this looks great!

@rwjblue rwjblue merged commit 66ea56f into emberjs:master May 24, 2021
@Windvis Windvis deleted the rfc-631/router-service-refresh branch September 24, 2021 19:27
@kategengler kategengler mentioned this pull request Nov 19, 2021
Closed
@jaredgalanis jaredgalanis mentioned this pull request Dec 30, 2021
Merged
6 tasks
@mydea mydea mentioned this pull request Jan 3, 2022
Closed
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@knownasilya knownasilya knownasilya left review comments

@rwjblue rwjblue rwjblue approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Windvis @rwjblue @knownasilya