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

Public Router service MVP #14805

Merged
merged 1 commit into from Jan 20, 2017

Conversation

Projects
None yet
10 participants
@scalvert
Contributor

scalvert commented Jan 9, 2017

This PR represents the MVP of the public router service described in this RFC. It incorporates work done by @locks in this PR.

It includes a minimal API:

  • currentRouteName
  • currentURL
  • location
  • rootURL
  • transitionTo

Status: Work in progress

Reviewers: @locks @rwjblue @ef4

Changes:

  • Added ember-routing-router-service feature flag
  • Added new router service and minimal API as described above
  • Added deprecation warning for use of private router service when feature enabled
  • Tests in progress

How to test drive this PR:

  • yarn start
  • Open up 2 tabs pointing to http://localhost:4200/tests/index.html, one with Enable Opt Features checked, ensure tests pass

TODO:
Write tests for

  • currentRouteName
  • currentURL
  • location
  • rootURL
  • transitionTo

RFC Implementation Phases

  1. MVP for public router service with minimal API
  2. Extended API, building on the work in phase 1
  3. Implementation of urlFor, ensuring correct performance profile
  4. Implementation of RouteInfo and associated API
@krisselden

Any API that doesn't meet the RFC here https://github.com/emberjs/rfcs/blob/master/text/0095-router-service.md#query-parameter-semantics shouldn't be included until it does.

@homu

This comment has been minimized.

Show comment
Hide comment
@homu

homu Jan 9, 2017

Contributor

☔️ The latest upstream changes (presumably #14806) made this pull request unmergeable. Please resolve the merge conflicts.

Contributor

homu commented Jan 9, 2017

☔️ The latest upstream changes (presumably #14806) made this pull request unmergeable. Please resolve the merge conflicts.

@locks locks requested review from rwjblue and ef4 Jan 9, 2017

@locks locks referenced this pull request Jan 9, 2017

Closed

[WIP] Router service #13868

@scalvert

This comment has been minimized.

Show comment
Hide comment
@scalvert

scalvert Jan 10, 2017

Contributor

Added tests for currentRouteName.

Contributor

scalvert commented Jan 10, 2017

Added tests for currentRouteName.

@scalvert

This comment has been minimized.

Show comment
Hide comment
@scalvert

scalvert Jan 10, 2017

Contributor

@krisselden I've stripped the urlFor implementation out of this review. We can work out how to proceed in a subsequent review.

Contributor

scalvert commented Jan 10, 2017

@krisselden I've stripped the urlFor implementation out of this review. We can work out how to proceed in a subsequent review.

@krisselden

This comment has been minimized.

Show comment
Hide comment
@krisselden

krisselden Jan 13, 2017

Member

I'd love @ef4 review to see if there is any concern about the timing of when these properties update as compared to the RFC, I know that the RFC was a chance to improve the router not just give it a prettier API.

Member

krisselden commented Jan 13, 2017

I'd love @ef4 review to see if there is any concern about the timing of when these properties update as compared to the RFC, I know that the RFC was a chance to improve the router not just give it a prettier API.

@rwjblue

This is looking really good! I'm glad you got to the bottom of that currentURL async issue we chatted about. 👏

I think the only real thing missing here at this point are some tests for .transitionTo, .rootURL, and .location properties.

Show outdated Hide outdated packages/ember-routing/lib/services/router.js
@@ -629,6 +633,7 @@ const EmberRouter = EmberObject.extend(Evented, {
let doUpdateURL = function() {
location.setURL(lastURL);
set(emberRouter, 'currentURL', lastURL);

This comment has been minimized.

@rwjblue

rwjblue Jan 13, 2017

Member

I think we should actually do this in the router.updateURL and router.replaceURL methods directly above the run.once(doUpdateURL) bit...

@rwjblue

rwjblue Jan 13, 2017

Member

I think we should actually do this in the router.updateURL and router.replaceURL methods directly above the run.once(doUpdateURL) bit...

This comment has been minimized.

@scalvert

scalvert Jan 13, 2017

Contributor

I can do that. My thought was I wanted to keep both set calls in the same place, but these methods are so closely related that it's find either way.

@scalvert

scalvert Jan 13, 2017

Contributor

I can do that. My thought was I wanted to keep both set calls in the same place, but these methods are so closely related that it's find either way.

This comment has been minimized.

@scalvert

scalvert Jan 13, 2017

Contributor

Ah, so it turns out that setting this after a scheduled call is what's fixing this issue. So, it looks like I have to leave it where it is.

@scalvert

scalvert Jan 13, 2017

Contributor

Ah, so it turns out that setting this after a scheduled call is what's fixing this issue. So, it looks like I have to leave it where it is.

set(router, 'currentPath', path);
set(router, 'currentRouteName', currentRouteName);
set(router, 'currentURL', currentURL);

This comment has been minimized.

@rwjblue

rwjblue Jan 13, 2017

Member

I would think this change is not needed since we are setting router.currentURL above in the changes to _setupRouter.

@rwjblue

rwjblue Jan 13, 2017

Member

I would think this change is not needed since we are setting router.currentURL above in the changes to _setupRouter.

This comment has been minimized.

@scalvert

scalvert Jan 13, 2017

Contributor

I thought this too, initially. It turns out that there's a different codepath whether you're visiting vs. transitioning, and we need this update to occur in both places. I think this surfaces part of @krisselden's concerns, where we want to take advantage, while we have the engine out on the floor, to refactor this to make it cleaner and more consistent.

@scalvert

scalvert Jan 13, 2017

Contributor

I thought this too, initially. It turns out that there's a different codepath whether you're visiting vs. transitioning, and we need this update to occur in both places. I think this surfaces part of @krisselden's concerns, where we want to take advantage, while we have the engine out on the floor, to refactor this to make it cleaner and more consistent.

access to the router.
@public
@class RouterService

This comment has been minimized.

@rwjblue

rwjblue Jan 13, 2017

Member

Should add the @category ember-routing-router-service flag here too (so that while the feature is pending it isn't listed in emberjs.com/api).

@rwjblue

rwjblue Jan 13, 2017

Member

Should add the @category ember-routing-router-service flag here too (so that while the feature is pending it isn't listed in emberjs.com/api).

This comment has been minimized.

@scalvert

scalvert Jan 13, 2017

Contributor

Done!

@scalvert

scalvert Jan 13, 2017

Contributor

Done!

@scalvert

This comment has been minimized.

Show comment
Hide comment
@scalvert

scalvert Jan 13, 2017

Contributor

@rwjblue thanks for the additional comments. I'm adding tests for the outstanding properties/method now - mainly taking it slow so I make sure I cover high value test cases.

Contributor

scalvert commented Jan 13, 2017

@rwjblue thanks for the additional comments. I'm adding tests for the outstanding properties/method now - mainly taking it slow so I make sure I cover high value test cases.

@scalvert

This comment has been minimized.

Show comment
Hide comment
@scalvert

scalvert Jan 13, 2017

Contributor

@krisselden I appreciate you wanting to make sure we're focussing on the right things with this PR, in terms of the opportunity to improve the router in general. This intent of this particular PR is to get a useful public API for the router in the hands of devs sooner rather than later. This will help provide immediate value, but does not preclude us circling back and refactoring parts of the internals later. In fact, that's our direct intention. We wanted to try to restrict these changes to simple, surface level changes before we get into the more meaty implementation, specifically the refactoring to use RouteInfo. Does this make sense to you?

I'll update the PR's description above to indicate the 4 phases, as discussed by @rwjblue and I.

Contributor

scalvert commented Jan 13, 2017

@krisselden I appreciate you wanting to make sure we're focussing on the right things with this PR, in terms of the opportunity to improve the router in general. This intent of this particular PR is to get a useful public API for the router in the hands of devs sooner rather than later. This will help provide immediate value, but does not preclude us circling back and refactoring parts of the internals later. In fact, that's our direct intention. We wanted to try to restrict these changes to simple, surface level changes before we get into the more meaty implementation, specifically the refactoring to use RouteInfo. Does this make sense to you?

I'll update the PR's description above to indicate the 4 phases, as discussed by @rwjblue and I.

@workmanw

This comment has been minimized.

Show comment
Hide comment
@workmanw

workmanw Jan 13, 2017

Contributor

This is super exciting. Thank you for taking this on and helping driving it home.

One additional thing I'd really like to see fall into one of the later phases is some mechanism for components to subscribe to willTransition and didTransition. We use that kind of thing so often in our app to prevent transitioning when data is dirty. I know the RFC talks about providing new hooks to do that and that might mean willTransition and didTransition have to wait until that part is flushed out. I just don't want to see these guys forgotten about.

Contributor

workmanw commented Jan 13, 2017

This is super exciting. Thank you for taking this on and helping driving it home.

One additional thing I'd really like to see fall into one of the later phases is some mechanism for components to subscribe to willTransition and didTransition. We use that kind of thing so often in our app to prevent transitioning when data is dirty. I know the RFC talks about providing new hooks to do that and that might mean willTransition and didTransition have to wait until that part is flushed out. I just don't want to see these guys forgotten about.

@locks

locks approved these changes Jan 13, 2017

@scalvert scalvert changed the title from [WIP] - Public Router service MVP to Public Router service MVP Jan 20, 2017

@rwjblue rwjblue merged commit 9cbf9f3 into emberjs:master Jan 20, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@MartinMalinda MartinMalinda referenced this pull request Apr 19, 2017

Merged

Handle /release requests #209

4 of 4 tasks complete
@nickiaconis

This comment has been minimized.

Show comment
Hide comment
@nickiaconis

nickiaconis Jun 29, 2017

Contributor

@workmanw The RFC introduces the routeWillChange and routeDidChange events in place of the willTransition and didTransition hooks/events, but I agree. I too would like to ensure they're on the roadmap.

Contributor

nickiaconis commented Jun 29, 2017

@workmanw The RFC introduces the routeWillChange and routeDidChange events in place of the willTransition and didTransition hooks/events, but I agree. I too would like to ensure they're on the roadmap.

@janmisek

This comment has been minimized.

Show comment
Hide comment
@janmisek

janmisek May 11, 2018

What about rest of features covered by RFC (RouteInfo, transition events ...) ? is there any subsequent issue to track it ?

janmisek commented May 11, 2018

What about rest of features covered by RFC (RouteInfo, transition events ...) ? is there any subsequent issue to track it ?

@locks

This comment has been minimized.

Show comment
Hide comment
@locks

locks May 14, 2018

Contributor

@janmisek There is not.

Contributor

locks commented May 14, 2018

@janmisek There is not.

@janmisek

This comment has been minimized.

Show comment
Hide comment
@janmisek

janmisek May 15, 2018

And are these features still planned for implementation ?

janmisek commented May 15, 2018

And are these features still planned for implementation ?

@ef4

This comment has been minimized.

Show comment
Hide comment
@ef4

ef4 May 16, 2018

Contributor

Yes, the RFC is merged which means anyone can PR the features. I started on several of them but had to back off to clear some other blockers first.

Contributor

ef4 commented May 16, 2018

Yes, the RFC is merged which means anyone can PR the features. I started on several of them but had to back off to clear some other blockers first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment