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

Align the mount and route APIs #722

Closed
wants to merge 1 commit into from
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 73 additions & 0 deletions text/0722-align-mount-route-apis.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
---
Stage: Accepted
Start Date: 2021-02-12
Release Date: Unreleased
Release Versions:
ember-source: vX.Y.Z
ember-data: vX.Y.Z
Relevant Team(s): Ember.js
RFC PR: https://github.com/emberjs/rfcs/pull/722
---

<!---
Directions for above:

Stage: Leave as is
Start Date: Fill in with today's date, YYYY-MM-DD
Release Date: Leave as is
Release Versions: Leave as is
Relevant Team(s): Fill this in with the [team(s)](README.md#relevant-teams) to which this RFC applies
RFC PR: Fill this in with the URL for the Proposal RFC PR
-->

# Align mount and route APIs

## Summary

Update the Ember `mount` API to accept a function argument which defines the Engine’s routes similar to how the `route` API works today.

## Motivation
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, it might be useful to explain what happens to pull the routes from within an engine itself (e.g. the customizations in ember-resolver for router-map), and why that is not ideal when using only routes defined within a single application/repo.


Some Ember applications utilize in-repo ember-engines strictly for their lazy loading and code organization features and not for their isolation features. In this context the isolation actually causes some maintenance overhead and prevents build optimization.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which kind of build optimization it has been preventing?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The way our code is structured we have 1 or more engine per team, most teams build and test their features in a single engine for their day to day work. During development teams rarely need any other engines outside of their own so the optimization we are working on allows teams to build only the engine that they work in regularly.

When we enable this optimization some things break such as building links from one routed engine to another since only one engine is built and no other engine routes are known at this point.

We need the ability to build links between routes because we have some features which require links to be built correctly, for example when a user performs a search we want to be able to save a link to the search results for use later. If those links cannot be built correctly it becomes hard to test that the code is working as expected until a full build and test is performed.

This change allows us to move links to the top level application thereby allowing link building via link-to or router.urlFor to work as expected.


If isolation is not a requirement and all engines in an application are in-repo it makes more sense to define all of the routes at the application level in the router.js.

## Detailed design

The required change will be made to the `mount` method in [packages/@ember/-internals/routing/lib/system/dsl.ts ](https://github.com/emberjs/ember.js/blob/b35106e4d8471e396eb1ee5a5044b6bbe72fa069/packages/%40ember/-internals/routing/lib/system/dsl.ts#L176). A third argument will be added matching the method signature of the `route` method.

```js
mount(name, options, mountCallback) {...}
```

If the argument is present and it is a function the route-map will not be resolved and the callback will be used in place of the resolved route-map.

```js
let engineRouteMap = null;

if (!mountCallback) {
engineRouteMap = this.options.resolveRouteMap(name);
}

if (mountCallback) {
mountCallback.call(childDSL);
} else if (engineRouteMap) {
engineRouteMap.class.call(childDSL);
}
```

## How we teach this
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you include some prose to be included in the updated Router.prototype.map API docs?

At the moment, it doesn't mention anything about this.mount at all (but it should).

https://api.emberjs.com/ember/3.24/classes/EmberRouter/methods/map?anchor=map


The alignment of the `route` and `mount` APIs should be easy to teach and is likely already inline with peoples mental model of the `route` API.

This change also unlocks a more idomatic way of using in-repo engines when the use-case does not require isolation.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the impact here to standalone engines?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ya, we should make sure to mention it. Overall the impact to stand alone engines is essentially none. We could suggest folks start leveraging the new callback to ensure the bundles are built properly (instead of "magically" hoisting engine-name-here/routes module into the host application).

Copy link

@villander villander Feb 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking better at this RFC, I'm not sure. Defines an engine's routing map from engines into router.js seems to force the host app to know things about the engine (internal implementation about how I'm structuring my engine). We must maintain the principle of isolation here.

We shouldn't confuse the hosting from mountPoint with the code isolation when the host app will know the engine implementation. Does it make sense @dgeb @rwjblue?

Looking at this old RFC it saying the routing map is already shared - https://github.com/emberjs/rfcs/blob/master/text/0010-engines.md#engine-boundaries

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking better at this RFC, I'm not sure. Defines an engine's routing map from engines into router.js seems to force the host app to know things about the engine (internal implementation about how I'm structuring my engine). We must maintain the principle of isolation here.

I don't think I agree with you here. A non-trivial number of ember-engines users (most of the applications that use ember-engines that I work on in fact) only include in-repo engines where there is effectively no benefit to this type of isolation. Additionally, there is no runtime isolation from the engine's routes (all routes are already required to be included up front in the applications own asset bundle).

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

only include in-repo engines where there is effectively no benefit to this type of isolation

I think it's still a kind of isolation where people aren't forced to separate your code using standalone engines. If some user wants to break your app into 2 engines and keep it as in-repo-engines I don't think it's bad and we shouldn't expose the engine routing map only because it is inside the /lib folder.

let me know if I'm missing something here @rwjblue you have been working on engines since the beginning, I may be missing something.


The Ember-Engines documentation will need to be updated to include instructions for defining engine routes via the mount API.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


## Drawbacks

One drawback is that this changes an existing public API and is something that existing Ember-Engines users may need to learn.

## Alternatives

Create an addon which extends the resolver to resolve route-maps from a custom location.
Loading