Add EngineRouterService to Engines

[villander]

It implements #587

• We're adding exact same API from RouterService, but adding an *External variant for all methods

[villander]

@rwjblue can we merge this?

[buschtoens]

This is yet missing the routeWillChange and routeDidChange events. I was playing around with my own EngineRouterService and found that in most cases, it's most sensible to transform the to and from properties of the Transition and expose the original RouteInfos as toExternal and fromExternal.

[villander]

@buschtoens how to do that? I did your solution, but it doesn't works -https://gist.github.com/buschtoens/de2faacfc8977e39843b7e2cc2f3fd1e#gistcomment-2960117

[buschtoens]

I think the missing routeWillChange and routeDidChange events are not required for the first iteration of this feature. Getting this reduced version the service shipped will already create tremendous value. We may also want to have a broader discussion about the ideal semantics of routeWillChange and routeDidChange — while I think that my proposal makes sense, some may disagree.

Regarding your comment on my gist: It looks like the event handlers are calling themselves recursively. This can have multiple reasons and I'd need to reproduce it.

[villander]

thanks @buschtoens, for while we'll push this without routeWillChange and routeDidChange events.
let me know when you figure out the recursive problem.

[lvegerano]

Will this affect engines that are getting the router service injected into them from the parent app?

[cinkonaap]

@lvegerano As the engine router is injected into service:router, I'd say yes it will.

However, I feel like this is an anti-pattern, as from how I understand it, Engine routing system shouldn't be concerned with host-app (that's why for example you pass the data via services).

[villander]

Will this affect engines that are getting the router service injected into them from the parent app?

Yes

However, I feel like this is an anti-pattern, as from how I understand it, Engine routing system shouldn't be concerned with host-app (that's why for example you pass the data via services).

This is not a anti-pattern, we just providing the same API that already exist on router to controllers, components and etc using a service. Nothing changes about isolation/concern. You can check this on my changes on this PR

[dgeb]

@villander getting close imo! I think we're down to needing a couple tests for the events above, and my main concern which is:

One of my major concerns here is handling the case when a user explicitly passes the parent router service to the engine. Seems like we should guard against the scenario when a service named router is passed.

We should probably have a hard error when a service named router is passed to avoid any potential confusion, and we'll need a version bump. BTW I think it's fine if the host's router service is passed but then aliased (say, as hostRouter).

[villander]

@dgeb thanks for bringing all concepts here.

I added all tests to EngineRouterService events, also I added the proper assert to avoid parent pass service named router to engines. Then they can pass using other names ; )

I think finally we're ready to move it forward and release it as v0.9.0

[dgeb]

@villander thanks for making those changes!

@rwjblue do you have time to do a final pass reviewing this?

[villander]

@rwjblue any chance to review it this week?

[dgeb]

This PR has dragged on for quite a while and I'd like to move it forward ASAP. That said, I was just doing another walkthrough here and could use some feedback from others in one area ...

I'd like to decide whether we really want to support all the *External methods on EngineRouterService (e.g. urlForExternal, isActiveExternal, etc.) We could instead tighten the API surface area by eliminating these methods and instead recommend direct usage of externalRouter on EngineRouterService (router.externalRouter.urlFor, router.externalRouter.isActive, etc.). With that said, it might be preferable to shorten externalRouter to just external to be more concise (router.external.urlFor, router.external.isActive, etc.)

@villander @rwjblue @buschtoens thoughts?

[villander]

This PR has dragged on for quite a while and I'd like to move it forward ASAP.

Yes, we're fighting with it for one year or more, and finally, we going to stabilize the Router on Engines. Looking forward to seeing it merged and released.

That said, I was just doing another walkthrough here and could use some feedback from others in one area ...

I agree we need more core team involvement here, this has been going on for years. Ember Engines seems "abandoned"

I'd like to decide whether we really want to support all the *External methods on EngineRouterService (e.g. urlForExternal, isActiveExternal, etc.) We could instead tighten the API surface area by eliminating these methods and instead recommend direct usage of externalRouter on EngineRouterService (router.externalRouter.urlFor, router.externalRouter.isActive, etc.). With that said, it might be preferable to shorten externalRouter to just external to be more concise (router.external.urlFor, router.external.isActive, etc.)

Seems a great decision to make Ember Engines more ergonomic with default methods from a normal Ember app and easy to update with RouterService coming from Ember.js repository, but we need to treat ember engines seriously and create an RFC for that, could you please do that @dgeb?

[buschtoens]

@dgeb I'm generally 👍 on your suggestion of removing *External methods in favor of an external pseudo-router. Having a consistent interface for engine-internal and engine-external routing would make adoption in addons and user-land code slightly less of a pain.

The name external / externalRouter could be confusing though. One might expect to find the host application RouterService.

I still believe that merging the internal and external namespaces as I suggested in #691 is the superior solution in terms of dev ergonomics. Another suggestion was using opaque route name via a {{external-route}} helper.

That said, router.external is something I could see myself using much rather than *External methods, even if not ideal IMO.

[dgeb]

Thanks for your thoughts, @villander and @buschtoens.

Looking forward to seeing it merged and released.

@villander Same here, but my hesitation is that this PR creates a whole lot of API surface area that is not strictly necessary. I'd like to push forward with the simplest possible version of this PR that provides the minimal API to ship an EngineRouterService.

The name external / externalRouter could be confusing though. One might expect to find the host application RouterService.

I think you're right here, @buschtoens. Since we're really talking about the router service on the root application, it would probably be more accurate to just call this router root. So you could access this root router as router.root.urlFor for instance.

My proposal to ship this PR as an MVP:

• Rename EngineRouterService#externalRouter to EngineRouterService#root
• Remove all the *External methods from this PR.

Does anyone have an issue with this as a starting point?

This can be followed quickly with a PR that deprecates the no longer needed *External methods on controllers and routes. Note that this is inline with the proposal to remove transition methods from controllers and routes in ember core (emberjs/rfcs#674).

We can also consider #691 more carefully after doing all this, since none of this will foreclose on that approach.

[buschtoens]

I think we're mixing up two things here. What's currently injected as externalRouter is, as you pointed out, the host application root router. Calling it root makes sense IMO. However, exposing it as public API may not be a good move, at least initially, as this breaks the engine encapsulation principle.

The *External methods do not allow to link to arbitrary host app routes. They only allow you to link to host routes that have been explicitly exposed to the engine via the dependencies.externalRoutes config.

My proposal to ship this PR as an MVP:

• Rename EngineRouterService#externalRouter to EngineRouterService#root
• Remove all the *External methods from this PR.

This can be followed quickly with a PR that deprecates the no longer needed *External methods on controllers and routes.

By doing just this we would break support for dependencies.externalRoutes.

So going by your suggestion what we'd need is:

• root: the host application RouterService, potentially (for now) as a private injection.
• external (for lack of a better name): a service / object that implements the RouterService interface (or a subset thereof) and allows linking to external routes (host app routes exposed via dependencies.externalRoutes).

The advantage of having this external RouterService-compliant object is that it's realtively pain-free for user-land code and addon code to access this.router.external instead of this.router, when e.g. the user passes an @external={{true}} argument. It would be more complicated to access differently named methods (*External) like currently.

I'd like to reiterate this point once more, as it seems that there was some confusion regarding the terminology of host routes (routes accessible via root) and external routes (routes exposed via dependencies.externalRoutes and accessible via external as aliases to host routes):

I still believe that merging the internal and external namespaces as I suggested in #691 is the superior solution in terms of dev ergonomics. Another suggestion was using opaque route name via a {{external-route}} helper.

[villander]

However, exposing it as public API may not be a good move, at least initially, as this breaks the engine encapsulation principle

I totally agree, it breaks the isolation principle even doing developer ergonomics easier removing surface API.

Also, I think we should open an RFC to discuss it better. There are many pros and cons here to be discussed. We already discussed some of them on the previous RFC and #691.

I still believe that merging the internal and external namespaces as I suggested in #691 is the superior solution in terms of dev ergonomics. Another suggestion was using opaque route name via a {{external-route}} helper.

I see one big pro:

1. No new API surface area, other than the external-route declaration itself

I see 4 big trade-offs:

1. non-obvious what engine and route we're linking
2. non-obvious when reading a route name whether it's internal or external
3. non-obvious to know where the route is defined as well (routes.js or engine.js).
4. Possibility of collisions mentioned by @gabrielcsapo - Merge internal and external route namespace, drop *External methods & <LinkToExternal> #691 (comment)

[buschtoens]

I see 4 big trade-offs:

1. non-obvious what engine and route we're linking
2. non-obvious when reading a route name whether it's internal or external
3. non-obvious to know where the route is defined as well (routes.js or engine.js).
4. Possibility of collisions mentioned by @gabrielcsapo - #691 (comment)

FWIW I addressed these concerns here: #691 (comment)

[villander]

FWIW, I replied to all of them here, addressing all objections #691 (comment) & #691 (comment)

[dgeb]

By doing just this we would break support for dependencies.externalRoutes.

Apologies. It's been so long since I've used engines personally that I completely overlooked current support for external routes. I will get back up to speed here and work through everyone's comments.

[villander]

@dgeb any update or forecast here? Is there something else I can do to move it forward?

[btecu]

Bump. This would be really nice to have.

[villander]

@btecu you can use this addon that provides this PR to Engines - https://github.com/villander/ember-engines-router-service