Conversation
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
There was a problem hiding this comment.
@nickschot sorry for the late review 🙈
My comments are mainly clarity/definition things.
|
|
||
| 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.
There was a problem hiding this comment.
So I think this is potentially a good thing to do, but I don't know if we technically need it, considering the focus of this RFC 🤔 this isn't really a user-facing API, it's really for framework developers and <1% of addon developers who would want to implement a route base class. The concept that we would need to explain here is the idea of Zebra-striping and not having to update all of your app at once to get new behaviour, and while this is implicit knowledge in the Ember community (and should be surfaced as explicit knowledge in at least a blog post) I don't know that the lack of this documentation would be a blocker for proceeding with this RFC 🤔
What do you think?
There was a problem hiding this comment.
What I think is that it could be seen as the same kind of ordering problem that we have in another comment below. After this paragraph, the next one says:
This RFC is not intended to describe APIs that Ember app developers would generally use, but it describes the low-level API intended for framework developers to develop next generation routing (...)
The idea of "next generation routing" can be enough to introduce the idea of "replacing the current router", but it needs to be positioned before. What I would probably do is describe the audience before the motivation, so if I am just a curious developer having a look at this page, I know this doesn't address me before I start the reading.
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.
text/1169-route-manager-api.md
Outdated
| 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 will unlock the possibility of implementing new Route base classes while also making it easier to replace the current router. | ||
|
|
||
| A concrete example: since it’s the Route that brings in the Controller, it will also, for example, become possible to implement a Route Manager that exposes a Routable Component without the need for a Controller. |
There was a problem hiding this comment.
does this also give us an opportunity to have a different way of defining query params?
how's that work right now?
query-params are allow-listed via the route-controller combo, yet it's the router that deal with them haha
There was a problem hiding this comment.
Yes, this opens the opportunity for us to have new query param implementations 🎉 we're being somewhat vague about the details here because (other than the legacy capabilities that we list) we don't want to over-scope this at the Route Manager API level 👍 we're confident that this API gives us enough flexibility to capture current behaviour and to experiment with new ones
|
|
||
| The `createRoute` method on the Route Manager is responsible for taking the Route’s factory and arguments and based on that return a `RouteStateBucket`. This is invoked by a Router. | ||
|
|
||
| ***Note:** It is up to the manager to decide whether or not this method actually instantiates the factory or if that happens at a later time, depending on the specific lifecycle the manager implementation wants to provide.* |
There was a problem hiding this comment.
e.g.: createRoute may return a function to be invoked later? (not sure that's a valid return value of createRoute (or rather, RouteStateBucket includes a function that could invoke a function that does await import or something -- I need to see when createRoute is called and what the value is used for 🤔 ))
There was a problem hiding this comment.
looks like it'll be sync, and getInvokalble, https://github.com/emberjs/rfcs/pull/1169/changes#diff-15112b55ff15b8563098e94dedd9f547dfe6d409bb6693aaca6dc7d8a71cbeb1R295, is where we'll manage async?
There was a problem hiding this comment.
yes that's correct. But also as it says in the alternatives, it's not strictly necessary that getInvokable() is async because we can manage that inside the lifecycle of the component but it's a nice convenience to have 👍
| `enter`: | ||
|
|
||
| - `beforeModel` hook | ||
| - `model` hook |
There was a problem hiding this comment.
is this also where the error event could happen? (this also bubbles)
(ie: returning a rejected promise / throwing during model)
There was a problem hiding this comment.
the error event would currently be an internal concern of the ClassicRouteMananger. It's not clear that we would have the same implementation for any future Route base class. Also it's worth remembering that the route manager would be calling the model hook on behalf of the router 👍
This is another case where we don't want to over-specify the Route Manager API to match 100% of the old behaviour because we want the flexiblity to change it in future.
I would imagine that any follow-up RFC that proposed a new Route base class would be very explicit about these details 👍
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