Conversation
NullVoxPopuli
added
S-Exploring
pichfl
force-pushed
the
nickschot/route-manager-api
branch
from
3c87662 to
b633f2e
Compare
|
I'm trying to better understand the design constraints behind the transitional Route Manager approach. What are the main reasons for introducing this intermediate layer first, instead of designing a new web-native router API directly? In particular, I'm curious which problems this staged approach is meant to de-risk, and what the main pitfalls would be if Ember tried to jump straight to a new router model. For example, are the biggest concerns around backwards compatibility, migration strategy, internal architecture, ecosystem impact, or something else? |
|
@alexraputa you summarized the reasoning behind this and many other RFCs for Ember very well in your question. The Motivation section reflects just that. We want to:
Directly ripping out and replacing the existing router API would go against these concepts. |
|
One thing I'm still curious about is the maintenance tradeoff: does introducing a transitional Route Manager risk expanding the router surface area for a long time, and if so, what are the intended exit criteria? I'm wondering how the team thinks about preventing an intermediate abstraction from becoming another permanent layer Ember Core has to support indefinitely. |
|
Are there specific capabilities of a future web-native router that this Route Manager is explicitly meant to unlock later? |
|
Not sure what a "future web-native router" will be, but the scope of this RFC doesn't cover anything like that. This is not the RFC dealing with a potential replacement. As stated, the goal is to increase the surface area and provide a clear and consistent interface that replaces the entangled situation we have without causing disruptions and allowing to iterate from there. As written in the motivation, it is an implementation detail targeted at maintainers of the framework. |
|
@pichfl thanks, that helps clarify the scope. |
|
@ef4 we have updated the RFC and applied all of your suggestions from last week 🎉 Please review again in the RFC meeting 🙏 |
BlueCutOfficial
left a comment
BlueCutOfficial
left a comment
There was a problem hiding this comment.
@nickschot sorry for the late review 🙈
My comments are mainly clarity/definition things.
|
|
||
| ## Motivation | ||
|
|
||
| The intent of this RFC is to implement a generic Route Manager concept so that we’re able to provide room for experimentation and migration to a new router solution. It aims to provide a well-defined interface between the Router and Route concepts. Well-defined in this case means both API and lifecycle. |
There was a problem hiding this comment.
Well-defined in this case means both API and lifecycle.
This sentence would deserve a bit more clarity. It should probably be something like "Well-defined in this case means that the interface includes both API and lifecycle". But just "lifecycle" remains a bit abstract as well. Maybe "control over lifecycle" if that's what it is about.
|
|
||
| The intent of this RFC is to implement a generic Route Manager concept so that we’re able to provide room for experimentation and migration to a new router solution. It aims to provide a well-defined interface between the Router and Route concepts. Well-defined in this case means both API and lifecycle. | ||
|
|
||
| This will unlock the possibility of implementing new Route base classes while also making it easier to replace the current router. |
There was a problem hiding this comment.
easier to replace the current router.
This should be accompanied by an explanation of why the current router would be a problem. I don't think this explanation should necessarily be in the RFC, it could rather be a linked to other resources that are dedicated to this.
|
|
||
| ### Determining which route manager to use | ||
|
|
||
| This follows the same pattern as existing manager implementations. This method will be used by the framework for the Route Base Classes it provides as well as by non-framework code wanting to provide their own Route Manager implementation. |
There was a problem hiding this comment.
existing manager implementations
This statement could be slightly more precise: existing where (Here we're talking about following the same pattern as the invokables that exist in Ember and Glimmer.)
|
|
||
| The `signal` is an `AbortSignal` provided by the Router which can be used to react to a cancellation of the current navigation. It can be passed to, for example, a `fetch` call. | ||
|
|
||
| `ancestorPromises` allows you to tie in to the asynchronous lifecycle of ancestor Routes. This opens the possibility for a RouteManager implementation for parallel resolution of the asynchronous lifecycle. The Classic Route Manager will rely on this behaviour to implement the current waterfall lifecycle. The ancestor promise will resolve with the `context` for that route i.e. in the Classic Route Manager that would be the return value for the `model()` hook. |
There was a problem hiding this comment.
allows you
Would replace "you" by who that could be in terms of role.
| } | ||
| ``` | ||
|
|
||
| Note: The current Route implementation has a different behaviour depending on if you are transitioning between two routes that are different, or if you are transitioning to the route you are currently on and changing any of the params for that route. This is an **internal concern** of the Route manager and will be implemented in the Classic Route Manager. We do not need to provide any `update()` hooks on the Route lifecycle to cater for this. |
There was a problem hiding this comment.
This is an internal concern of the Route manager and will be implemented in the Classic Route Manager
The difference between Route Manager and Classic Route Manager hasn't been defined clearly somewhere above. I know you go into more details below, but a rough idea should be introduced somewhere before you use that term.
|
|
||
| ### Route lifecycle update hooks | ||
|
|
||
| A previous iteration of this RFC provided explicit `willUpdate()`, `update()`, and `didUpdate()` hooks in the Root Manager interface that were distinct to the `enter()` related hooks and would only be called when you are entering the same route you are currently on with a transition. This was added to simplify the implementation of the Classic Route Manager, which is intended to encapsulate the current behaviour of Ember's existing routes. In reality the trade-off between making the implementation easier and having a wider API surface area is most likely not worth it. |
There was a problem hiding this comment.
Is it really Root with a big "R"?
| }; | ||
| ``` | ||
|
|
||
| For the Route Manager API we will rework this structure to a generic invokable. This way the manager implementation can decide how render happens and what arguments are passed. Deferring render while waiting on asynchronous behaviour (like the Classic Route model hooks) will be a Route Manager concern. |
There was a problem hiding this comment.
Very optional, but I wonder how comfortable the community currently is with the concept of "routes as invokables".
ef4
left a comment
ef4
left a comment
There was a problem hiding this comment.
Only one significant question from RFC review today. If the suggestion doesn't work for compatibility, perhaps you can expand on the strategy for getting interop with the existing loading route behaviors. My understanding is that they're out of scope for this RFC because they're an inter-route concern and would be dealt with at the whole-router ("orchestrater") level.
| import type { ComponentLike } from '@glint/template'; | ||
|
|
||
| interface RouteManager { | ||
| getInvokable: (bucket: RouteStateBucket) => ComponentLike; |
There was a problem hiding this comment.
Two concerns with getInvokable:
- It's synchronous, which means we can't experiment with route managers that load their components dynamically.
- It's detached from the lifecycle. A big reason to do asynchronous work in
enteris so that your components never have to see a state where that work isn't finished (you can use the route to absorb asynchrony).
Can we eliminate getInvokable and instead change the return type of enter to Promise<{ context: unknown; invokable: ComponentLike >}? The motivation is that until both the data (context) and code (invokable) are ready the route isn't ready to render anything.
| This RFC proposes 3 groups of hooks for lifecycle management of a Route. | ||
|
|
||
| - `enter` - called when a route is visited. | ||
| - `update` - called when the input for a route has changed, think dynamic segment or query param. |
There was a problem hiding this comment.
It seems that this was missed by the recent removal of the update hook.
| - `update` - called when the input for a route has changed, think dynamic segment or query param. |
8 participants
Propose Route Manager API
Rendered
Summary
This pull request is proposing a new RFC.
To succeed, it will need to pass into the Exploring Stage, followed by the Accepted Stage.
A Proposed or Exploring RFC may also move to the Closed Stage if it is withdrawn by the author or if it is rejected by the Ember team. This requires an "FCP to Close" period.
An FCP is required before merging this PR to advance to Accepted.
Upon merging this PR, automation will open a draft PR for this RFC to move to the Ready for Released Stage.
Exploring Stage Description
This stage is entered when the Ember team believes the concept described in the RFC should be pursued, but the RFC may still need some more work, discussion, answers to open questions, and/or a champion before it can move to the next stage.
An RFC is moved into Exploring with consensus of the relevant teams. The relevant team expects to spend time helping to refine the proposal. The RFC remains a PR and will have an
Exploringlabel applied.An Exploring RFC that is successfully completed can move to Accepted with an FCP is required as in the existing process. It may also be moved to Closed with an FCP.
Accepted Stage Description
To move into the "accepted stage" the RFC must have complete prose and have successfully passed through an "FCP to Accept" period in which the community has weighed in and consensus has been achieved on the direction. The relevant teams believe that the proposal is well-specified and ready for implementation. The RFC has a champion within one of the relevant teams.
If there are unanswered questions, we have outlined them and expect that they will be answered before Ready for Release.
When the RFC is accepted, the PR will be merged, and automation will open a new PR to move the RFC to the Ready for Release stage. That PR should be used to track implementation progress and gain consensus to move to the next stage.
Checklist to move to Exploring
S-Proposedis removed from the PR and the labelS-Exploringis added.Checklist to move to Accepted
Final Comment Periodlabel has been added to start the FCP