Skip to content
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

Closed
wants to merge 2 commits into from

Conversation

trentmwillis
Copy link
Member

@chadhietala
Copy link
Contributor

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:

App = Ember.Application.extend({
  modulePrefix: config.modulePrefix,
  podModulePrefix: config.podModulePrefix,
  Resolver,
  engines: ['blog', 'settings']
});

Or if the engines have settings

App = Ember.Application.extend({
  modulePrefix: config.modulePrefix,
  podModulePrefix: config.podModulePrefix,
  Resolver,

  engines: {
    blog: {
      dependencies: {
        services: [
          'store',
          {'session': 'user-session'}
        ]
      }
    },
    settings: {
      dependencies: {
        services: [
          'store',
          {'session': 'user-session'}
        ]
      }
    },
  }
});

Internally you could just do Object.keys(this.engines) to derive the list. For the host app there is an implicit mainApp. By being explicit about the engines in the app we can then special case them for issues around lazy loading.

@trentmwillis
Copy link
Member Author

it definitely feels like "magic syntax" that is a gotcha.

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.

I could imagine that the host app holds the linkable contexts and are exposed to the engines so that you could do something like...

While I think this would work for a large majority of cases, what would occur when your Engine mounts another Engine at the route mainApp and you also want to be able to link out to the Engine? We could special case mainApp as a route/mount name to make it off limits, but this issue could pop-up anywhere that a linkable context's mount point collides with another Engine's internal route. This introduces development constraints that shouldn't exist given the fact that Engines should be mostly insulated from their consuming application.

For the host app there is an implicit mainApp.

What would mainApp mean if your Engine is mounted within another Engine? Would it point to the highest parent or would it point to the most immediate parent? If you intend for your Engine to always point to the direct parent, how would you accomplish this?

By being explicit about the engines in the app we can then special case them for issues around lazy loading.

I'm curious as to what those issues are as async is definitely a primary concern here.

@dnachev
Copy link

dnachev commented Feb 18, 2016

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 link_to_parent becomes feasible as it will be always one level (of course, it should be renamed to link_to_external or something like this).

@trentmwillis
Copy link
Member Author

I think relying on knowing the context tree to be able to link outside of the current context is too limiting.

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 ^.foo limits you to having a parent context and a route named foo, but nothing more. If you explicitly link to context foo, then your application needs to have a parent context, an additional context, and the additional context must be named foo. What happens if the consuming app later decides that the functionality at foo would be better suited as a simple route instead of another Engine?

Also, it hinders reusability of engines, which rely on each other. You cannot take an engine and put it different application.

Perhaps I could be wrong, but reusing an Engine in multiple applications should be relatively trivial in a large number of cases using the ^ syntax. If your dependency is only that "you go one context up" then it doesn't matter what that context is, just that it is there. This lends itself to the idea that Engine's shouldn't really be aware of their mounting which makes them easier to reuse.

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.

My suggestion is to not think of engines as a tree, but as a flat list (requiring unique names of the engines)

This runs into similar naming collisions as mentioned in response to @chadhietala's suggestion. What happens if you mount a blog engine in more than one context of your application? You could namespace them, but you're then back to relying on a context tree, but this time it is an explicitly named context tree.

@dnachev
Copy link

dnachev commented Feb 19, 2016

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 link-to-context "orders" 'order.index' ... It doesn't care where the orders engine is currently mounted, etc. It knows it needs to go to the orders engine specific URL. I think this plays nice with the fact that orders engine will have to provide information regarding the fact that this product has already been ordered and the exact order, which was used. The discussion in the engines RFC seems to indicate there will be a way to reference services from an engine. I would say the same solution found for the multiple instances of engines for the consumption of services and other parts of an engine can be applied here.

@trentmwillis
Copy link
Member Author

you are using the route structure for providing "API" (note the quotes) for linking to other functionality in the application.

Yes, that is currently the primary goal/focus of this RFC.

how can you build applications out of engines and have them communicate in fluent and flexible way.

I'm not sure I agree with this as I don't think links/urls should be the primary method of communicating between Engines.

It doesn't care where the orders engine is currently mounted, etc.

But for linking to orders where it is mounted is important, is it not? Consider for instance if Amazon had different pages for orders based on if you are on a Business or Personal account. They might use the same Engine since they share functionality, but perhaps for product reasons they chose to use /business/:id/orders and /account/:id/orders as the URLs for the different types of accounts. Which one will you wind up at if you just link to orders?

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 ^ syntax, the latter requires a "unique identifier" which is the primary issue I see with it.

I can see use cases both for wanting to link to a specific place and to a specific thing. Does this seem reasonable?

@dnachev
Copy link

dnachev commented Feb 20, 2016

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 ^ syntax, the latter requires a "unique identifier" which is the primary issue I see with it.

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.

@diogomafra
Copy link

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:

  • The engine declares required urls.
  • The application provides urls using an abstraction (here it's using the function urlFor).
  • The application links to external urls using that abstraction (using the get-url helper).
  • It would be possible to provide a base url an concatenate sub-urls (e.g. {{link-to concat-url(get-url('help'), 'users')}}, or simply get-url('help', 'users'))

@diogomafra
Copy link

Maybe we can provide URLs for the engine in the same way we provide services.

Here is a quick hack demonstrating how it could work: ember-engines/ember-engines#71

@trentmwillis
Copy link
Member Author

Maybe we can provide URLs for the engine in the same way we provide services.

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 externalRoute dependencies like so:

// 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}} as a method of utilizing those links:

{{#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:

  • Using route instead of url as the nomenclature helps denote that it still uses the same old routing,
  • I think urlFor, get-url, and concat-url are unnecessary abstractions since we're only dealing with route names, and
  • Introducing link-to-external helps keep us from muddying the behavior of link-to, especially since external routes will be defined in a different location from normal routes.

I see this as having the following benefits:

  • We're now decoupled from the context tree since we don't attempt to traverse anything (@dnachev's complaint)
  • We don't need to worry about naming collisions since everything we reference is internal (my complaint)

But, it has the following drawbacks:

  • Introduces multiple new APIs (albeit, they're all simplistic)
  • Requires configuration from the consumer

@chadhietala
Copy link
Contributor

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 externalRoutes at build time as part of Ember CLI. However this seems like it could be prototyped in user-land code and potentially folded in later with the conventional API.

@diogomafra
Copy link

@trentmwillis I agree with everything you said, except with the introduction of the link-to-external component.

I think it's better to provide a simple helper that returns the route name. For example: {{#link-to (external-route "new-user") }}Create{{/link-to}}. This allows us to reuse the existing link-to component, and thirty party components like the ember-href-to.

This may lead to a large amount of configuration for engines that have many entry points from the consuming app. ( @chadhietala )

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.

@trentmwillis
Copy link
Member Author

@diogomafra I'm not convinced the external-route helper will be very beneficial. The reasoning for this is that link-to will become scoped to the engine by default, this means that if you did {{link-to (external-route "new-user")}} it will attempt to scope the value returned from external-route to the current engine.

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.

link-to-external gives us that escape valve without tying it into the DSL. It will be able to reuse the majority of the existing link-to component, just ignoring the scoping semantics. Since any addons dealing with links will need to be updated to take into account these scoping semantics, I think it makes more sense to just adopt a secondary construct in Ember and introduce similar constructs into any addons.

@dfreeman
Copy link
Contributor

@trentmwillis I'm a fan of your most recent proposal for externalRoutes config – it's roughly what I've been imagining in the free moments I've had to think about this, and I like that it feels consistent with the existing configuration story today in terms of wiring up services.

I'm somewhat on the fence about the utility of @diogomafra's proposed external-route helper. In a world where they're driven by static configuration, I can think of three approaches for how an external route could be addressed from an engine:

  1. just use the existing {{link-to}}/transitionTo, and make it an error to declare a route within an engine that has the same name as a declared external route
  • pros: no new API surface area, other than the external route declaration itself
  • cons: non-obvious when reading a route name whether it's internal or external
  1. introduce (external-route)/externalRoute to return a wrapper object denoting a route that isn't local to an engine (similar to htmlSafe strings in templating)
  • pros: composeable – works anywhere you're ultimately feeding a route name into a transition, regardless of whether it's part of Ember proper, other 3rd party code, or your own app logic
  • cons: pretty magical, and it requires new developers to grok the concept of an external route as a concrete reified thing
  1. introduce {{link-to-external}}/transitionToExternal as new first-class APIs
  • pros: explicit at the point of invocation, and a logical extension of existing functionality
  • cons: largest amount of new API surface area to document and teach; requires new parallel constructs in all other locations dealing with routing (e.g. the routing service proposal)

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.

@trentmwillis
Copy link
Member Author

  1. .... cons: non-obvious when reading a route name whether it's internal or external

I would also add it is non-obvious to know where the route is defined as well (routes.js or engine.js). For those reasons and the possibility of collisions, I don't think this is a very good option.

  1. ... pros: composeable – works anywhere you're ultimately feeding a route name into a transition... cons: pretty magical, and it requires new developers to grok the concept of an external route as a concrete reified thing

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.

  1. ... cons: largest amount of new API surface area to document and teach

I don't believe this will be much more to document and teach than external-route. It will potentially be less actually since we don't introduce any new representations for routes.

@trentmwillis
Copy link
Member Author

Updated the RFC to reflect feedback and the new approach.

@dgeb
Copy link
Member

dgeb commented May 3, 2016

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.

@rwjblue
Copy link
Member

rwjblue commented Nov 8, 2016

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...

@trentmwillis
Copy link
Member Author

I agree, would like to upstream this. I think one thing that we keep running into is teachability/naming. The -external part seems to trip developers up, but I think most of this stems from not thinking of Engines as isolated parts of the application. Not sure if that should stop this from moving forward though.

@rwjblue
Copy link
Member

rwjblue commented Apr 28, 2017

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.

@knownasilya
Copy link
Contributor

Has the {{link-to (external-route "new-user")}} syntax been accepted as well?

@Serabe
Copy link
Member

Serabe commented Feb 28, 2018

@rwjblue this RFC has been in FCP for 9 months. Anything holding it back?

@rwjblue
Copy link
Member

rwjblue commented Mar 9, 2018

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.

@villander
Copy link

villander commented Apr 20, 2020

@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.

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

Successfully merging this pull request may close these issues.

10 participants