-
-
Notifications
You must be signed in to change notification settings - Fork 407
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
Linking Between Engines (and Applications) #122
Conversation
I've never been a fan of micro-syntax 😁. While it's not "yet another API", from outsiders point of view it definitely feels like "magic syntax" that is a gotcha. I'm sort of wondering why we can't just be explicit. In Rails Engines linking must be done explicitly. <%# Will like to the main_app's root_path %>
<%= link_to 'Home', main_app.root_path %>
<%# Will like to the engines root_path %>
<%= link_to 'EngineHome', root_path %> I've never used Rails Engines, so I don't know what the downsides are. I'm just stating there is prior art that potentially could be leveraged. While a lot of Ember is convention over configuration, I feel like some of these "power user" features don't necessarily need to follow that paradigm at first, but rather figure out where the cow paths are at then pave them. I could imagine that the host app holds the linkable contexts and are exposed to the engines so that you could do something like. {{!-- Will like to the mainApp's index --}}
{{link-to 'Home' 'mainApp.index'}}
{{!-- Will like to the engine's index --}}
{{link-to 'EngineHome' 'index'}} From the host app one could declare the engines as so:
Or if the engines have settings
Internally you could just do |
I agree that it is somewhat "magicky", but that little bit of magic (which I don't feel is a huge conceptual hurdle) gives us added flexibility.
While I think this would work for a large majority of cases, what would occur when your Engine mounts another Engine at the route
What would
I'm curious as to what those issues are as async is definitely a primary concern here. |
I think relying on knowing the context tree to be able to link outside of the current context is too limiting. A big case, where this is a problem is when refactoring an engine and putting it in different context requires all dependencies to be updated. In a complex applications, where engines are supposed to be used, it's probably really hard to do it. Also, it hinders reusability of engines, which rely on each other. You cannot take an engine and put it different application. This might not be such a big problem as engine reuse is probably limited case, but it would be a good to have if we ever need it. My suggestion is to not think of engines as a tree, but as a flat list (requiring unique names of the engines). This would allow an engine to depend on another and their exact position in the hierarchy is easily changeable to allow refactoring. When flat list is used instead of tree, the |
I don't disagree that being tied to a context tree would be limiting in some ways; however, I don't necessarily think that is a bad thing as it gives flexibility in other ways. For instance, defining a link to a sibling engine of
Perhaps I could be wrong, but reusing an Engine in multiple applications should be relatively trivial in a large number of cases using the On the flip, if you explicitly say "you go to this context" (as you seem to be suggesting), then reusing Engines becomes more difficult as you are now tied to an explicitly named context and so you will need to make sure wherever you mount the Engine has that context available. I also tend to believe that Engines which need to link into a specifically named context are most likely not going to be candidates for reuse.
This runs into similar naming collisions as mentioned in response to @chadhietala's suggestion. What happens if you mount a |
It seems what you are trying to accomplish is that you can link to routes, which are not defined by the current context. There are no assumptions regarding who will serve this route - essentially, you are using the route structure for providing "API" (note the quotes) for linking to other functionality in the application. If this is the ultimate goal of the spec, then I agree that what you are suggesting is a good approach. However, I think it goal needs to be broader - how can you build applications out of engines and have them communicate in fluent and flexible way. The use case is the following: Let's say Amazon has been built with Ember Engines. One engine deals with product browsing - it deals with rendering products, searching etc. At some point, the user stumbles on a product, which he has already purchased. The user should be able to click on a link to get him to the actual order. The product browser engine should only specify |
Yes, that is currently the primary goal/focus of this RFC.
I'm not sure I agree with this as I don't think links/urls should be the primary method of communicating between Engines.
But for linking to All that said, assuming you can work out any collision/duplication issues, I think we may actually want both of these constructs. The solution I've proposed (as you've pointed out) is concerned with "linking to a place", the solution you're proposed is concerned with "linking to a thing". The former requires a "unique path" which is what we get with using the I can see use cases both for wanting to link to a specific place and to a specific thing. Does this seem reasonable? |
This is pretty good summary of the two sides. I don't think product browser engine should make decision whether it wants to go to the business or the personal accounts. This is most probably decided by the orders engine itself as the product browser should not have any understanding of the different types. I think the construct for addressing specific instance should be the same as used for consuming services from specific engine instance instead of using the mount point of the engine. |
I think we need a better abstraction for URLs inside Ember. The browser uses URLs as simple strings, but we can't do that because we need more information. Maybe we can provide URLs for the engine in the same way we provide services. // Engine declaration
export default Engine.extend({
dependencies: {
services: [
'store',
],
urls: [
'new-user',
'help'
]
}
}); // Application using that engine
App = Ember.Application.extend({
engines: {
emberBlog: {
dependencies: {
services: [
'store'
],
urls: [
'new-user': urlFor('users.new'),
'help': urlFor('help.blog')
]
}
}
}
}); {{! links to external urls }}
{{#link-to get-url('new-user')}}Create user{{/link-to}}
{{#link-to get-url('help')}}Help{{/link-to}} So, we have:
|
Here is a quick hack demonstrating how it could work: ember-engines/ember-engines#71 |
Seems similar in spirit to what @dnachev is thinking. After thinking about it more, I think I like the idea you present @diogomafra , with a few modifications. The Engine can declare // Engine declaration
export default Engine.extend({
dependencies: {
externalRoutes: [
'new-user',
'help'
]
}
}); The application then provides simple route strings to configure the engine: // Application using that engine
App = Ember.Application.extend({
engines: {
emberBlog: {
dependencies: {
externalRoutes: [
'new-user': 'user.new',
'help': 'help.blog'
]
}
}
}
}); Then, finally, we introduce {{#link-to-external "new-user"}}Create New User{{/link-to-external}} Most of the changes are minor from what you propose, but I think the naming and usage changes are important for these reasons:
I see this as having the following benefits:
But, it has the following drawbacks:
|
As mentioned before I think it's a good idea to punt to configuration at first to see where the patterns are at and overtime a more conventional API may come around. The downside of the configuration story is that all routes are opt-in instead of opt-out. This may lead to a large amount of configuration for engines that have many entry points from the consuming app. Another way of limiting the configuration is to create |
@trentmwillis I agree with everything you said, except with the introduction of the I think it's better to provide a simple helper that returns the route name. For example:
The engine can accept a "root" route and concatenate the remaining path. For example, if the engine needs a path to "users.new", "users.list" and "users.show", it can request the configuration for the "users" route and concatenate "new", "list" and "show". This obviously makes the app more coupled with the engine, but following a structure defined by the engine makes the configuration easier. Of course, this is a decision that every engine has to make, to use a specific route for everything or accept a "root route" and force the app to follow a defined structure. |
@diogomafra I'm not convinced the Therefore, we would need to build in some sort of escape valve in the route DSL to denote when a route path should be looked up as an external route instead of being scoped to the engine. That escape valve would introduce another (most likely private) aspect to the route DSL and feels a bit "magical" to me personally.
|
@trentmwillis I'm a fan of your most recent proposal for I'm somewhat on the fence about the utility of @diogomafra's proposed
It may be that there are other alternatives out there that prove better, but among these three it seems like each has interesting tradeoffs, and I'm not certain which is best. |
I would also add it is non-obvious to know where the route is defined as well (
I would also add this introduces a new way of representing a route, as an object instead of just as a string. This affords us some flexibility but also means any route-handling functionality will now need to be polymorphic, which leads to more complex code and (potentially) performance issues.
I don't believe this will be much more to document and teach than |
Updated the RFC to reflect feedback and the new approach. |
The implementation proposed in this RFC has been merged into ember-engines via ember-engines/ember-engines#75 We will leave this RFC open as we try out the implementation in the addon. Thanks again to @trentmwillis for pushing this forward. |
Where are we at with this? Discussion seems to be mostly quieted down, and I believe that folks are successfully using the implementation in ember-engines addon. @dgeb / @MiguelMadero / @nathanhammond / @trentmwillis - Are more changes needed here or are we ready to try to advance to FCP? Ultimately, I would like to land so that we can upstream the overrides we have in ember-engines for this... |
I agree, would like to upstream this. I think one thing that we keep running into is teachability/naming. The |
We discussed this during todays Ember core team meeting today, and we are 👍 on this moving forward. Given that there is little conversation here, and the core team is in favor I an moving this forward into the final comment period. |
Has the |
@rwjblue this RFC has been in FCP for 9 months. Anything holding it back? |
Discussed this a bit with @tomdale and we decided that we should close this. Not as "wontfix" but more as "already done" in addon space without needing more detailed core framework involvement. This is absolutely public API from ember-engines point of view, and it will be supported as such. @trentmwillis - Thank you very much for your hard work on this, and I'm very sorry that we let this sit for sooo long. |
@rwjblue @dgeb @locks @jenweber this PR was closed wrongly because it already was implemented on ember-engines as noticed on this PR by Dan - #122 (comment). Then I think it should be part of the ember RFC merged since it already implemented in ember projects. |
Rendered.