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

Conversation

jsturgis
Copy link

@jsturgis jsturgis commented Feb 12, 2021

This RFC describes the motivation and implementation for
aligning the mount and route API signatures.

emberjs/ember.js#19402

This RFC describes the motivation and implementation for
aligning the mount and route API signatures.
@jsturgis jsturgis changed the title RFC to align the mount and route APIs Align the mount and route APIs Feb 12, 2021
Copy link
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

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

Thanks for working on this!

IMHO, this is a nice simplification (both for developers' mental models, and for the build pipeline).

/cc @dgeb @villander @buschtoens

}
```

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


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.

@rwjblue rwjblue added T-framework RFCs that impact the ember.js library T-routing labels Feb 12, 2021
@rwjblue rwjblue self-assigned this Feb 12, 2021

## Motivation

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.


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

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.


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.

@meirish
Copy link

meirish commented Feb 19, 2021

This may be out of scope, but it feels related to ember-engines/ember-engines#80 (though I realize the inside-out nature of this adds complexity).

@wagenet
Copy link
Member

wagenet commented Jul 23, 2022

Is there still interest to move this forward?

@villander
Copy link

villander commented Jul 25, 2022

IMO we should discuss and see if it's still relevant and have some chance to be included on #819

cc/ @runspired @SergeAstapov

@wagenet wagenet added the S-Proposed In the Proposed Stage label Dec 2, 2022
@wagenet wagenet assigned ef4 and unassigned rwjblue Jan 27, 2023
@ef4
Copy link
Contributor

ef4 commented Jan 27, 2023

This RFC is probably not aligned with what we want to do. It's motivation says:

Some Ember applications utilize in-repo ember-engines strictly for their lazy loading and code organization features and not for their isolation features.

Going forward, these are not relevant reasons to use engines, because we have better alternatives for both lazy loading and code organization.

  • for lazy loading, you can use import() via either ember-auto-import or embroider
  • for code organization, first-class component templates means you can use all the organization patterns that work with normal ES modules.

@achambers
Copy link
Contributor

achambers commented Sep 1, 2023

@SergeAstapov / @void-mAlex Do you have any thoughts on this?

@SergeAstapov
Copy link
Contributor

@achambers I strongly agree with what Ed said in previous comment and I'm not sure I see the actual problem this gonna solve as Engines by design are separate apps and they should not leak anything up to the host app.

@wagenet wagenet added the FCP to close The core-team that owns this RFC has moved that this RFC be closed. label Sep 22, 2023
@wagenet
Copy link
Member

wagenet commented Sep 22, 2023

Given the fact that this is not aligned with our current direction we are moving to FCP to Close.

@achambers
Copy link
Contributor

Looks like no comments so this should be good to close. Will leave to the RFC #2 meeting to do so.

@ef4 ef4 closed this Sep 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FCP to close The core-team that owns this RFC has moved that this RFC be closed. S-Proposed In the Proposed Stage T-framework RFCs that impact the ember.js library T-routing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants