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

[RFC] Provide externalRouter to Engines #779

Open
2 of 4 tasks
villander opened this issue Jun 20, 2021 · 5 comments
Open
2 of 4 tasks

[RFC] Provide externalRouter to Engines #779

villander opened this issue Jun 20, 2021 · 5 comments

Comments

@villander
Copy link
Member

villander commented Jun 20, 2021

Detailed design

Regarding the #587 and the PR already implemented #669 as a team we discussed and agreed in one of our monthly meetings on February 12 of this year to follow these steps:

  • Deprecate current usage of router:service() by registering a stub service that proxies router calls to the root app router service - Add deprecation warning to router service from host #764

  • Create a deprecation documentation page about the router service on ember-engines.com. Provide suggestions for how to register the host's router as hostRouter and/or the root router as appRouter. However, we don't want to automatically create either of these services in an engine - the decision should be the developer's. - Start deprecations page with host router service ember-engines.com#111

  • Create engine-specific external router and make it available on-demand for the users

  • Consider allowing an experimental build flag to allow usage of an alpha engine-specific router which we can iterate upon.

We want a shorten externalRouter to just external to be more concise than support all the *External methods. For example: we would like to have - router.external.urlFor, router.external.isActive and etc.

To do that, we need to map all externalRoutes allowed and avoid the router.external to access prohibited routes

// super-blog/addon/engine.js
export default class SuperBlog extends Engine {
  // ...
  dependencies = {
    externalRoutes: [
      'home',
      'settings',
    ]
  };
}

Also, we need to add a flag to enable this feature on-demand:

// super-blog/index.js
const { buildEngine } = require('ember-engines/lib/engine-addon');

module.exports = buildEngine({
  name: 'super-blog',
  lazyLoading: {
    enabled: true
  }
  ...
  externalRoute: {
    enabled: true,
  }
});

Usage

Option A) External namespace

import Route from "@ember/routing/route";
import { action } from '@ember/object';
import { inject as service } from '@ember/service';

export default class YourRoute extends Route {
  @service router;
  
  @action
  goHome() {
    this.router.external.transitionTo('home');
  }
});

Option B) Route helper

import Route from "@ember/routing/route";
import { action } from '@ember/object';
import { inject as service } from '@ember/service';
import { external } from 'ember-engines/helper'

export default class YourRoute extends Route {
  @service router;
  
  @action
  goHome() {
    this.router.transitionTo(external('home'));
  }
});

We must keep the eyes on this RFC that deprecates the no longer needed *External methods on controllers and routes. - emberjs/rfcs#674

Drawbacks

This RFC introduces the new concept of engines, which increases the learning curve of the framework. However, I believe this issue is mitigated by the fact that engines are opt-in packaging around existing concepts.

In the end, I believe that "engines" are just a small API for composing existing concepts. And they can be introduced at the top of the conceptual ladder, once users are comfortable with the basics of Ember, and only for those working on large teams or distributing addons.

Alternatives

Keep the old mindset from the first Engines RFC and move forward with the #669 adding *External methods.

Unresolved questions

Further, consider an external routing DSL to replace *External suffixed methods. Odds are that any solution will require a framework-level RFC since it will need to be understood by framework-level concerns such as LinkTo.

cc: @dgeb @rwjblue @buschtoens

@dgeb
Copy link
Member

dgeb commented Jul 9, 2021

Thanks for keeping progress on the router moving, @villander.

I'm unclear on how this proposal intersects with:

Consider allowing an experimental build flag to allow usage of an alpha engine-specific router which we can iterate upon.

Are you seeing the proposal in this issue (i.e. router.external) as separate from the alpha router we discussed experimenting with?

My impression was that the alpha router would be used to experiment with all engine-specific routing concepts, including external routes.

@villander
Copy link
Member Author

villander commented Jul 9, 2021

Are you seeing the proposal in this issue (i.e. router.external) as separate from the alpha router we discussed experimenting with?

No! it's the same, we going to release the engine-specific (i.e. route.external) with the build flag as an experimental feature. It's only a breakdown

@dgeb what I understood so far is that we going to use engine-specific including all external routes passed down to Engines. Since the engine-specific router, like the *External methods will have access only for externalRoutes as usual. Is that ok for you?

@dgeb
Copy link
Member

dgeb commented Jul 9, 2021

No! it's the same, we going to release the engine-specific (i.e. route.external) with the build flag as an experimental feature. It's only a breakdown

Gotcha, thanks for clarifying.

So I think we really have two alternatives on the table for handling external routes:

A) The one described in this RFC, in which an external namespace on the router service contains alternative external-route-specific methods.

B) An external route helper (or perhaps just external: string prefix) for namespacing external routes that can be used directly with the engine's router service and the standard LinkTo helpers.

Perhaps the best path forward will be to fill out option B as an RFC similar to this one, and at the same time fill in any gaps here for option A. Hopefully, we'll then be in a good position to decide if one approach is clearly better. If so, we can implement that behind the experimental flag. If not, perhaps we implement them both for comparison purposes in separate branches and then decide?

It will be good to talk this over in our monthly meeting and hear from @rwjblue, @buschtoens, and others who are interested in this space.

@lvegerano
Copy link

Will this allow the engine router to route to external routes without the necessity of mapping external routes. Our app has a ton of routes. We use engines to share the same experience across multiple routes. Initially we started mapping external routes but this created a bit bloat in the code for little benefit as we do not use engines as a "separate app".

So back to the question will this allow us to

this.router.external.transitionTo('home.foo.bar')

where home.foo.bar is a route in the consuming (parent) app?

@villander
Copy link
Member Author

villander commented Aug 6, 2021

Will this allow the engine router to route to external routes without the necessity of mapping external routes

It's not true Luis! As you can see on the proposal the routes continue to be declared on dependencies as externalRoutes.

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

No branches or pull requests

3 participants