-
-
Notifications
You must be signed in to change notification settings - Fork 408
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
Route actions #394
Route actions #394
Conversation
Add initial rfc draft Fixes Fixes Fixes Fixes Fixes
eceec99
to
3583973
Compare
Personally, I think we should try to figure out how to remove controllers entirely, based off this RFC: So, if just the regular |
I agree, especially with the push towards closure actions, basically forcing actions to be on the controller is not ideal IMHO. I use |
this may turn in to a philosophy discussion, but most of my actions are in components, because I it's a nice adherence to single responsibility. putting all the actions at the controller/route level feels messy / bloated. |
It makes sense that most of your actions and anyone's actions are in the components. That's fine and expected. The problem is that you never need an action in the controller, it gives you nothing 99% of the cases (the 1% is the query params) because it can easily be in a (wrapper) component while at the same time if you need an action in the route (which is not that rare actually), then you need the controller as well just to bubble it. |
I also think that it does not necessarily matter in this case - there are valid arguments to put actions in the controller, the route, or a component. But it's a fact that actions in the route are basically unusable with closure actions right now - and I don't think that makes any sense. |
I also use ember-route-action-helper in practically all of my apps. However, I've seen a lot of devs use it directly in component templates, which in my opinion is a real footguny thing to do, because you logically couple the component to a certain route hierarchy. If we make this helper "official", I think we should also disallow its usage in any template that is not a route template. |
We might also want to think about exposing a primitive to traverse the templates route hierarchy, so that addons like ember-route-task-helper (same thing for tasks) also don't need to rely on these hacks / private API. |
I think it's fine to call it from components' templates? If your component is gonna be used across multiple routes, then you just move that route action higher in the route hierarchy. Also it's more and more rare that we have plain (route) templates, usually everything come from components and in that case if we add that helper but cannot call it from a component it doesn't help us at all. You will either need to inject it from a parent top-route component all the way down which seems clumsy, or need to catch the action from the top-route component and resend to route which essentially will do what controllers do now. The idea is to simplify the actions towards routes and let the user decide how to use it, right ? |
In my case, I actually put all business logic actions in the controller. So that just leaves my components responsible for anything UI related and my route for resolving the data derived from the URL. To elaborate more, there's a concept of Container and Presentational Components. I like to think of my controllers as container components. So I'm on the side of not wanting to push the route-action for my selfish reason of I don't need it and it might just add more to the learning curve. |
that's a good compromise
I think this is a good thing to have, but I don't think it should traverse outside of the immediate route. @rmmmp,
I use this as well but I don't think the implementation is separated at the entirety of components and the controller. In https://gitlab.com/NullVoxPopuli/emberclear/, you can take a look at how the packages/frontend/src/ui/routes folder is structured with local lookup / private collections, and how the presentational / container stuff is somewhat achived there. It's also kind of a fuzzy concept, in that just the template of an ember component most strongly correlates to the presentation component in React. so... 🤷♀️
we are going to be at-odds then, cause it's my goal to see if I can remove controllers from ember altogether, and see if we can simplify the mental model of data flow. :)
This feels foot-gunny to me. How would you debug an issue with this if you have a component 10 levels deep? |
I don't get it, how is debugging related to where (=in which route) your route action is ? @rmmmp I don't get what controllers can do that route-top-level components can't at the moment in your architecture, apart from query params. Disallowing patterns that don't create any hussle to us, doesn't seem like a good idea. Maybe if we had complexity issues in the codebase I would understand it. But it feels like we are forcing the user to our current design pattern that work for us but not everyone thinks like us. |
@vasilakisfil seems like mainly to encourage good architecture. As another compromise, what about allowing usage anywhere but make a template linting rule that is enabled by default to discourage usage outside a controller template? That way someone could still opt-out of the rule explicitly, but they would know it's not a recommended path.. |
@NullVoxPopuli can you elaborate more on this?
Also I'm not against of removing controllers and I'm actually in favor of it provided that there's a clear migration plan. When that happens, it's hopefully just moving those logic into a top-level component. But as of right now, they're here so I'm using it for that purpose. @vasilakisfil Apart from query params, nothing really. But they're here so I'm maximizing it. When the time comes, I can migrate it to a route-top-level component. |
I have been in frameworks since I don't know when and patterns that used to be "good" have turned out really bad after some years, same thing has happened with Ember a couple of times. It's good to put some conventions in frameworks but adding too many constraints makes things too narrow, diminishes innovation and makes a framework work only for specific people (mostly with the same way of thinking). And again: if it put us constraints on complexity on codebase that doesn't justify it, I would get it. But just adding a constraint because it's a bad pattern now or in the common case, doesn't seem fair to me. |
I think putting For me, the ideal way would be to keep things in the addon and figure out what needs to be done with controllers first. I haven't been following any discussions with controllers lately but from when the idea of removing it popped-up to now, I'd say it's a long overdue one. |
@rmmmp
I see containers as components that contain the bulk of logic / whatever for a singular particular behavior I don't like the idea of having controllers be the container, because it implies you have a ton in there, and that all your components are template-only. :-\ Just seems like a weird way to spread out concerns to me.
This is just everyone learning together, yeah? more experience? more shared knowledge?
I guess where I'm struggling with this is just that it seems premature in relation to my own personal goals for the controller. Ideally just
that sounds fair :) |
Interesting. I've always looked at that scenario as single-file component vs multiple-files, not JS = container and HBS = presentation. For me, template-only-glimmer component = functional components in React. From the article I posted earlier:
When I translate that to Ember, to me it says: Are written as template-only-glimmer components unless they need state, lifecycle hooks, or performance optimizations. |
IMO this is the wrong way to look about moving logic. Controllers are much more of a 1-1 mapping to component and it is much easier to understand and replace controllers with components directly. Creating a
IMO this is an anti-pattern that was shortly adopted but is not heavily ubiquitous. Using routes as a stateful model is really dangerous and has downsides. In general I think route actions are a pattern that is a temporary patch for the router service and better documentation around services. |
For me it boils down to the question of "why are you putting these actions in the routes" vs controllers or container style components?
This is often the case when an action from child routes needs to act on shared state between multiple routes. This is better handled by having a service to hold this shared state and then calling actions that explicitly work with this service (this also has the benefit of being shared across the application instead of needing to arbitrarily move the action up and down the route tree
This leads to a mix of problems; one of which is that it often leads to referring to
Controllers are a stable and consistent API in Ember, while there are discussions to move to a more component/service architecture this is still a while a way and there will be legacy support for some time. In this process services with a container component are more closely related to controllers and less likely to cause future issues than moving state back into the route. |
I meant for query params. There is nothing else you can't do with a component and I think we all understand this.
and
If only services could have actions.. but until then what you describe seems quite more complex to me than just sending an action the route. Also, the same service you describe could easily be injected in the routes. There is no need to hold state (and you shouldn't really) in the routes. Actions in routes (and everywhere else) are just pure functions, i.e. stateless. Is there something I misinterpret regarding the definition of "stateful" ? In any case, would you like to elaborate on the solution you describe that uses the new router service ? Because it's the only real alternative suggested in this thread but I can't quite understand how it's better.
You shouldn't deal with the controller in that way as they will be removed at some point, that's for sure. Sure, in the old times, we used to do that but that's irrelevant now. Instead, you should use services to handle/propagate these changes.
Again I am not sure why having actions in routes make them stateful. |
We try to follow a pattern: actions interacting with the API are defined at the route level, so
I think having a default solution to improve this case would be a good addition. Either using the |
I think the path forward should just be to keep route-action in an addon. |
Here is a twiddle showing a component that is using the router service and Based on your comments on saying that your route actions are stateless, then any action that you currently have in a route should be able to be moved into components.
Directly calling out to services from templates would be more confusing than the small amount of code that it would save. I meant that in your component you would gather any locally scoped values and pass them along to a function call on the service. class MyComponent extends Component {
@service mySharedLogicService
@action
onSubmit(ev) {
ev.preventDefault();
this.mySharedLogicService.saveData(this.formData);
}
} |
For what it's worth, I think the separation between components and routes are a good thing and my current thinking is that I'd be sad to see them rolled into some new I see the route/component pair as similar to
With this in mind, to me, actions live on controllers and are not a concern of the route (aka view model in MVVM). |
Ideas, feel free to add to list or claim! - [x] I've been getting a lot of questions about how tree-shaking is coming along. I would be willing to train anyone that wants to help on what's already done and what still needs to be done. Disclaimer: It's a lot of work! https://twitter.com/kellyselden/status/1050717338595745792 (🔏 @jessica-jordan) - [x] emberjs/rfcs#389 (🔏 @kennethlarsen) - [x] emberjs/rfcs#394 - [ ] Hacktoberfest roundup? - [x] #30DaysOfEmber https://twitter.com/PoslinskiNet/status/1054446639719608320 (🔒 @chrisrng ) - [x] CodeSandbox for Ember (https://twitter.com/CompuIves/status/1057681015299366912) (🔒 @chrisrng )
I have all my actions in controller. Controller owns the data (model), so it also should own the actions which work with that data. I see route as a delegate to setup the controller and that’s it. In some rare cases I might use route action but this soon comes down to “this.controller” mess. Which to me is a sign that my route is not the owner of the data and therefore my action is in the wrong place. |
I’m disinclined to agree with the claim the routes are stateless singletons meant to serialize/deserialize the state. If this were so then there’d be no need for routes at all in a world where using higher order components to manage data fetch actions was the norm (but in a routeless world we’d find ourselves back in the routable components debate debacle) This argument that we don’t want controllers but we also want to narrow what routes are responsible for would force developers into some of the worst habits I see today as their “recommended path”: namely how to manage mutations to a route’s model correctly. Mutations must be signaled to the route for re-serialization regardless, and having a route be responsible for initial load but a controller/component responsible for reload / mutation seems incredibly disjointed. |
This. |
but, why does putting all data mutations on the route make sense? |
I think we agree that the deserialization process happens in the So where exactly would the serialization of application state happen? Some kind of persistence action in the route, right?
But here, I understand that that persistence action should belong to the controller, as it's co-located closer to the state being mutated (data owner)
Finally, it is suggested that actions would better just belong to components. So no actions in controllers? Thoughts @tomdale ? I find all this confusing. |
All data mutations, or the actions to interact with the store (i.e. My approach so far has been to place all calls to the store (such as
You mean placing interactions with the store in a service? Services don't have the actions hash. What about "actions up"? |
To be clear, I am not arguing that you shouldn't use services, mutate data, or fire network requests from components. Nor am I arguing that only routes can/should interact with the store (a service) or records (data in the form of application-level state). What I am arguing is that if you are mutating, reloading, or changing out the data that the route provided via the |
this is the fundamental thing that we're debating about this RFC.
my pattern works out really well for productivity as well. I do not need to go look for anything, cause it's all right next to the thing I'm modifying.
DDAU is not meant for the entire component layer and how it interacts with the controllers route. Also, as far as 'the actions hash', I don't need it -- but I'm also using native classes and sparkles-components. for methods on services that I'd want to call, for now, I just make a proxy method/action in my component. Most of the time these component methods have some sort of validation, or starts a task, which the task then calls the service. |
Agreed on DDAU. I'm happy that that setup works well for you, but I don't think it should be recommended as it depends on several add-ons and features not available in Ember today. What would you recommend to a developer that started with Ember today? Right now, it boils down to "whatever works for you" and this detracts from Ember's primary goals. I can't find an official stance, even if I suspect the answer is "the controller". The guides and even the super rentals app don't have an example of where (and why) should a beginner place a call to |
I'm inclined to disagree. 😉 Architecturally, there's nothing wrong with components fetching and mutating model data. Even if a route has a single "root" model, app UIs often decompose that root model into related "sub-models." An illustration of what I mean, from the Travis CI UI: UI is inherently coupled to its underlying model data. Components are responsible for decomposing UI into smaller discrete units, so they are also responsible for decomposing the associated model data. Routes represent asynchronous boundaries between logical sections of an application. Today the asynchrony comes from model data, but with code-splitting, it also includes code and other assets as well. We want the route to be the "chokepoint" where we pause to wait for the minimum set of resources needed to render a section of the app in a meaningful state. Given this framing of routes and components, fetching the model in the route can be seen as a performance optimization, so that critical model data can begin to be fetched in parallel with code. Otherwise, we'd need to wait for the JavaScript assets for a route to load just to start fetching model data. Routes should be simple and stateless so that we may include them in the initial application payload. They should contain the minimum set of behavior and configuration needed to start bootstrapping a route when it is requested. All other route-specific code should live in a separate bundle. We don't believe that routable components, or "async components" that have a more declarative way of fetching their data dependencies, are a flawed idea. It's an idea we'd like to revisit in the future, once improvements to the existing fundamental APIs have landed.
Model mutations that need to be serialized to the URL should be rare. Generally only a model's unique ID is serialized in the URL (e.g. Fetching and mutating models often goes hand-in-hand, but I don't see them as inherently coupled operations. For example, imagine a GraphQL query that contains deeply nested models. In this case, the root-most component sends the query to the server and gets back the data as a single JSON payload. The component peels off top-level model data and passes it to child components, which themselves peel off the next layer of model data, and so on down the hierarchy. However, when it comes time to mutate a model, the operation likely doesn't belong at the root-most component that did the fetching. That's because GraphQL mutations generally operate on a single model, and don't require reserializing the entire graph, just the fields of the model you want to mutate. In this case, it would be most appropriate to put the mutation-handling behavior in the component responsible for that specific model you're editing.
By application state, I'm interpreting this to mean the kind of thing you'd put in a query param; things like currently selected sort order, for example. I think we need a more "action-like" API for query params that is in line with DDAU. Components would be able to invoke this action to mutate query params, just as they are able to transition to different routes and models today.
Correct. Long-term, we do not see controllers as part of the programming model. There are reasons we need them around today, like query params, but we're designing for a world where they're not necessary.
All actions are closure actions that live in components. Model mutation should happen at the root-most component that "knows" about that model. It's fine for the root-most component to mutate a model, even if it was fetched by the route. The one unfortunate exception to this rule is query params. In that case, you are still going to need to use a controller. As mentioned above, we have a pressing need for a query params API that is compatible with DDAU.
Agreed, and this is something we desperately need to fix as part of Octane. |
We discussed this at the core team meeting today and, given the new feedback above that came in after the FCP announcement, we wanted to answer that and give time for further discussion. However, even taking into account the most recent comments, we still had consensus that this RFC was not appropriate to merge at this time. Pending new substantive issues, we still intend to close this RFC after another week. |
Why is action bubbling considered "bad architecture"? One of the things that we have used the bubbling of "route actions" before was to keep logic around an action in a single place. For instance, in a classic "todo" type of app, you have a index and a show for a single item. If this item can be deleted from both, putting the "delete" action on the base route (that the index and show routes are sub routes of) has been useful, cause it creates a single place to store the logic around removing an item (sending analytics, additional cleanup, ensuing authorization, etc). There is no state in this instance, the item to be manipulated on is passed to the action. If you are doing something like checking that the have the privileges to perform the action in the route, that doesn't seem to be the state you are referring to, cause this awareness is something that the route normally does (is even in the guides). If having these types of actions on the route is discouraged, would these types of "logic clusters" get put into a service that then gets injected into the various controllers and components? |
@webark You can either use a service or a provider component to handle this. As this is just a delete operation and you don‘t need to juggle state around a component that yields a „remove todo“ action would be good enough. Action bubbling is hard to understand and you have to hunt for the actual code being executed by checking all points of the route hierarchy that could be handling the action. Of course using route actions can work as well as action bubbling. Imo there are better ways to do things though and I have seen bad patterns emerge from using route actions ( Tldr: everything what Tom said in his post ;) |
Initially there would be no shared component between a show and an index. So adding a wrapping tagless component or service to handle these actions is the prescribed pattern?
Route depths of more then 4 total levels from application to calling route are rare. That is not many places to check, and it has a very clear progression.
I think this is the main place where cognitive dissonance has occurred in the community. “Asynchronous” i feel is the key word there. Everyone would probably agree that routes are logical separations, but it appears that one side views it as a broader “data” boundary rather then just a “minimum set of resources needed to render a section of the app in a meaningful state”. Let’s use the travis ci example for restarting/canceling an individual job. The ui (and potential data) breakdown here is “build -> group -> job”. Where i would put this action would be on the build route. The logical reasoning for this is matching up of where the data is coming down from, to where the action is going up to. The main build’s state is dependent on all its nested jobs. It needs to reference things like the overall status of the build, the running total of completed jobs, etc. So when a individual job is effected, the place where this change of data will ultimately need to be reflected is at the build layer, so the action reaching up and living in that same layer makes sense to me. If the action that restated/canceled the job stoped right at the individual job component, there would be a disconnect where the action only goes up to the test layer, but then relies on the store to ferry that change back up to the build layer, so that the data can start moving down. The reason this would live in the main build route rather then the build show controller, would be twofold. One, i need to share this base action from the build show route, test show route, even the build index route (if they had some kind of quick view of running jobs), and i want to put this on one place at the same level where the data is originating from. Two, since i view the route as a “data boundary” rather then just mearly a asynchronous boundary, i want to describe “this is the data that is needed, and the actions you can take on that data” in the same place. @tomdale is this reasoning a flawed reading of “DDAU” methodology, and is opening up the route as a concept that encompasses the data needed in general rather then simply the bare asynchronous side of fetching the data fit with the long term goals of where Ember is going? (code splitting, lazy loading of a routes components and assets, asynchronous components, etc) |
I'm very happy to hear the route's role articulated with such clarity by the core team. It would be wonderful for this to eventually make its way into the docs. We've (EmberMap) found much success in thinking of routes in this way. It encourages developers to think closely about what those minimum needed resources are, and then nudges them to declaratively specify those resources in the form of a Declaratively specifying a page's minimum data needs in the route
and more. So, needless to say I am very much in favor of this being the mental model going forward!
@webark I would agree with this approach if the |
@samselikoff agreed. And that would be how the data is ferried, just congnativly I haven enjoyed having those live alongside each other at the same level. (though since using more and more concurrency tasks, that separation has already occurred, and have found it to be unfortunate from a mental model (at one point i defined a task in a route and bound it to the controller in the routes init, and after 🤢 my lunch decided to not go down that route (pun intended))) Actions, that i prefer to be in the route, are not purely proxies for data mutations though. They often hold the shared logic of sending analytics or other similar tracking triggers, rollback and error handing logic, authorization checks, etc. Where have you found it to be most beneficial to put these shared set of concens? |
These days I would probably use a Provider for that. I will also sometimes add methods to models, e.g. |
If the reason we want to keep the routes as clean as possible is the forthcoming features in Ember (code splitting, lazy loading etc) then for me that's fine. But if it's just because it's a bad pattern I think the tiny code footprint this RFC adds does not justify it given how many people use that pattern. |
@vasilakisfil I think that's the primary motivator for me personally, and I realize now is something we've never explicitly communicated in the past. Code-splitting will introduce a new constraint into the system, and in this case I think it means the role of the Now, if the core team we felt very strongly that actions on the route were critical to productivity, we'd probably want to explore the solution space to see if there was a way to keep their benefits around, even with the new constraint of code-splitting. I'll be the first to admit that there are cases where route actions are a very, very elegant way of modeling certain problems, particularly mutations to the route's model that can be triggered by multiple UI elements in the same route. However, we have to balance this against the common complaints that:
I suspect there may be a way to bring back the benefits of route actions in the future, without as many downsides. Off the top of my head, interesting ideas to explore would be:
Note that I'm not particularly for or against any of these ideas, as they haven't been properly explored. The important thing is that I agree there's a problem to solve here and that we haven't even begun to exhaust the solution space. In terms of priority, we want to first pay down the complexity of the existing action systems and land highly-anticipated features like code-splitting, and see where those changes leave us. Once those are in place, I think that that is the appropriate time to start thinking about new patterns or abstractions that might fill the void left by route actions. Until then, I'll be satisfied if there's a high-quality addon that can be used by teams who really value the route actions pattern. That's the goal of unlocking experimentation—people can scratch their own itch without having to wait around for the core team. |
@tomdale that is reasonable. Thanks. 😊 Two questions :
|
This is throwing deprecations as of Ember 3.6. It’s not that widely-used in the application and route actions aren’t that favoured in the RFC discussion, so we decided to remove it. emberjs/rfcs#394
I do have some extra conversation here since I've recently been working on features in our app that heavily use I'm still of the opinion that On that subject I think that a future RFC should look at removing routes from the action handler stack for clarity with the In maintaining the route actions in our code, it is clear that this pattern has a lot of footguns and problems. The only area where we do rely on route actions that can not be directly migrated to controllers or components is use of the |
Thanks for the enlightening discussion, everyone! We discussed this at the core team meeting today and still have consensus that this is not an RFC we want to pick up at this time. We would like to simplify the existing action system and land code-splitting first, and see where that leaves us. If there are still common cases that are ergonomically painful, I think that would be the appropriate time to start exploring new APIs that fill the niche that route actions fill today. To that end, I will close this RFC for now. Thank you @vasilakisfil for all the work you put into this; it's really appreciated and sparked a much-needed conversation. |
@tomdale I often refer back to this thread and your thoughts and the related discussion around how to think about role of a route in an Ember app's architecture, specifically, this description:
I wondered if, after 4 years and route splitting still being in-progress with It seems to me that since there has been no public progress on breaking QP's out of controller-jail (I think @NullVoxPopuli had some experiments on this though), and also no public progress in routable components / a general alternative system to controllers - so I assume there's little evolution of the ideas presented in this thread and it's mostly all still valid in (almost) 2023? |
@lougreenwood We're going to be revamping routing as part of Polaris. We'll be reviewing past routing RFCs as part of that process and I'll add this to our list. Thanks! |
rendered.