From 260d449fb018e0df3bebca2206c4876327075aa6 Mon Sep 17 00:00:00 2001 From: Sergey Astapov Date: Thu, 21 Apr 2022 14:49:35 -0400 Subject: [PATCH 1/4] Public RouterService --- text/0000-template.md | 188 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 188 insertions(+) create mode 100644 text/0000-template.md diff --git a/text/0000-template.md b/text/0000-template.md new file mode 100644 index 0000000000..f48801a729 --- /dev/null +++ b/text/0000-template.md @@ -0,0 +1,188 @@ +--- +Stage: Accepted +Start Date: +Release Date: Unreleased +Release Versions: + ember-source: vX.Y.Z + ember-data: vX.Y.Z +Relevant Team(s): Ember.js +RFC PR: +--- + + + +# Public `RouterService` import + +## Summary + +This RFC proposes adding public import for [`RouterService`](https://api.emberjs.com/ember/release/classes/RouterService) +making it extendable by addons. + +## Motivation + +By providing public import for `RouterService` and making it extendable, +[`ember-engines`](https://github.com/ember-engines/ember-engines) would provide extended version +of the `RouterService` to the user defined Ember Engines. +This change allows to fulfill the existing gap when it comes to navigating between Ember Engine and host app +*and* solve Ember v4 compatibility issue that exists in `ember-engines` today. + +Proposed changes would allow to deprecate `` +component provided by `ember-engines` package. As a result, there will be less +engine specific concepts and code would be portable between regular app and Ember Engines. + +Once this RFC gets implemented, following will be possible in user defined Ember Engines: + +* links to external routes in templates within the engine rendered via `` component: + + ```hbs + Go Somewhere + ``` + +* using `RouterService` methods for redirects to the external routes: + + ```javascript + # addon/components/my-component.js + export default class extends Component { + @service router; + @action clickLink () { + this.router.transitionTo('external-route'); + } + } + ``` + +There is another problem this RFC seeks to solve. +Authors of large scale applications often create an addon(s) to contain reusable components +that can be rendered either in the context of the host app or Ember Engine. +In such scenario an extra guard needs to be introduced as `` component +available only in the context of Ember Engine and does not exist in the context of regular application, +so it's common to see such pattern: + +```hbs +{{#if @isEngine}} + Contact Us +{{else}} + Contact Us +{{/if}} +``` + +Once this RFC gets implemented, above snippet could be replaced with + +```hbs +Contact Us +``` + +## Detailed design + +### `ember-source` package + +#### `` component + +The use of `-routing` service in `` component needs to be replaced with `router` service. +Currently, those properties being pulled from `-routing` service: + +* `currentState` +* `currentRouteName` + +Currently, those methods of `-routing` service used: + +* `generateURL` +* `transitionTo` +* `isActiveForRoute` + +#### `RouterService` + +Provide public import via `@ember/routing/router-service`. + +### `ember-engines` package + +Provide `router` service that extends from `@ember/routing/router-service`: + +```javascript +# ember-engines/addon/services/router.js +import RouterService from '@ember/routing/router-service'; + +export default class extends RouterService { + // ... +} +``` + +The extended `RouterService` should override make all public methods and properties play nice with Ember Engines citizen. + +> This is the bulk of the RFC. + +> Explain the design in enough detail for somebody +familiar with the framework to understand, and for somebody familiar with the +implementation to implement. This should get into specifics and corner-cases, +and include examples of how the feature is used. Any new terminology should be +defined here. + +## How we teach this + +Ember Engines documentation provided at [ember-engines.com](https://ember-engines.com) +should be updated with `` replacing all the references of `` component. + +Transition path needs to be published on [ember-engines.com](https://ember-engines.com) containing following steps: + +1. Replace use of `` component + with `` in every engine codebase. +2. If `ember-engines-router-service` was installed and used, replace any of it' usage with + `RouterService` provided by `ember-engines`. This should be done in the same PR + updating `ember-engines` to the version that ships `RouterService`. + +> What names and terminology work best for these concepts and why? How is this +idea best presented? As a continuation of existing Ember patterns, or as a +wholly new one? + +> Would the acceptance of this proposal mean the Ember guides must be +re-organized or altered? Does it change how Ember is taught to new users +at any level? + +> How should this feature be introduced and taught to existing Ember +users? + +## Drawbacks + +> Why should we *not* do this? Please consider the impact on teaching Ember, +on the integration of this feature with other existing and planned features, +on the impact of the API churn on existing apps, etc. + +> There are tradeoffs to choosing any path, please attempt to identify them here. + +## Alternatives + +* Utilize `@ember/legacy-built-in-components` to keep `` + a thin wrapper around legacy implementation of `` component. + + As this may be considered temporary solution to solve Ember 4 compatibility for `ember-engines`, + this is not viable long term solution as any changes made to `` component in the Ember.js codebase + will need to be backported to `@ember/legacy-built-in-components`. + +* Re-implement `` functionality in `ember-engine`. + + This is not viable long term solution as any changes made to `` component in the Ember.js codebase + would need to be backported to `ember-engines`. + +* Deprecate and remove `externalRoutes` mapping. + + This would require providing *full route name* to `` component *including* engine mount point + within the engine code. + This option is not viable as it breaks isolation principle of Ember Engines + whereas every engine should be self-scoped micro-frontend. + +* Introduce `{{path-for}}` Engine Router helpers. + + This option was discussed in [RFC #806 "Engine Router Helpers"](https://github.com/emberjs/rfcs/pull/806) + which was considered as partial solution to teh problems discussed in this RFC. + +## Unresolved questions + +None. From 3d84a3f1755c6b841680f7abe453b4a80ec6c0dd Mon Sep 17 00:00:00 2001 From: Chris Thoburn Date: Thu, 21 Apr 2022 12:40:37 -0700 Subject: [PATCH 2/4] Weekly Meeting Review Changes --- text/0000-template.md | 103 +++++++++++++++++++++++++----------------- 1 file changed, 62 insertions(+), 41 deletions(-) diff --git a/text/0000-template.md b/text/0000-template.md index f48801a729..b1c1c1b556 100644 --- a/text/0000-template.md +++ b/text/0000-template.md @@ -1,6 +1,6 @@ --- Stage: Accepted -Start Date: +Start Date: 2022-04-21 Release Date: Unreleased Release Versions: ember-source: vX.Y.Z @@ -24,20 +24,24 @@ RFC PR: Fill this in with the URL for the Proposal RFC PR ## Summary -This RFC proposes adding public import for [`RouterService`](https://api.emberjs.com/ember/release/classes/RouterService) -making it extendable by addons. +This RFC proposes adding a public import for [`RouterService`](https://api.emberjs.com/ember/release/classes/RouterService) +making it extensible by addons, removing the external link assertion from `` and rebuilding the `` component +to rely entirely on the public routing service instead of the internal `-router`. ## Motivation -By providing public import for `RouterService` and making it extendable, -[`ember-engines`](https://github.com/ember-engines/ember-engines) would provide extended version -of the `RouterService` to the user defined Ember Engines. -This change allows to fulfill the existing gap when it comes to navigating between Ember Engine and host app -*and* solve Ember v4 compatibility issue that exists in `ember-engines` today. +Today, users of [`ember-engines`](https://github.com/ember-engines/ember-engines) are required to utilize separate +custom components, helpers and services for linking. This leaves the experience feeling sub-par, in addition to introducing +extra maintenance friction attempting to keep these primitives aligned with those in `ember-source`. -Proposed changes would allow to deprecate `` +By providing public a import for `RouterService`, `ember-engines` would be able to provide its own router service +implementation extending from the base implementation. Combined with minor changes to the engines-specfici behavior of +`link-to`, this change would allow us to fill the existing DX gap when it comes to navigating between an Engine +and a host App *and* allow engines to eliminate use of the legacy components required to support 4.x versions of Ember. + +These proposed changes would further allow `ember-engines` to deprecate the `` component provided by `ember-engines` package. As a result, there will be less -engine specific concepts and code would be portable between regular app and Ember Engines. +engine specific concepts and code would be more portable between regular applications and engines. Once this RFC gets implemented, following will be possible in user defined Ember Engines: @@ -59,12 +63,14 @@ Once this RFC gets implemented, following will be possible in user defined Ember } ``` -There is another problem this RFC seeks to solve. -Authors of large scale applications often create an addon(s) to contain reusable components -that can be rendered either in the context of the host app or Ember Engine. -In such scenario an extra guard needs to be introduced as `` component -available only in the context of Ember Engine and does not exist in the context of regular application, -so it's common to see such pattern: +**Bonus: improved DX for addons** + +This RFC seeks to provide a solution that also solves another DX issue faced by engines developers. + +Authors of large scale applications often create an addon(s) to contain reusable components that can +be rendered either in the context of the host App or Engine. In such scenarios an extra guard needs +to be introduced as the `` component is available only in the context of an Engine +and does not exist in the context of regular applications, so it is common to see patterns such as the following: ```hbs {{#if @isEngine}} @@ -86,7 +92,7 @@ Once this RFC gets implemented, above snippet could be replaced with #### `` component -The use of `-routing` service in `` component needs to be replaced with `router` service. +The use of `-routing` service in `` component would be replaced with the `router` service. Currently, those properties being pulled from `-routing` service: * `currentState` @@ -98,13 +104,22 @@ Currently, those methods of `-routing` service used: * `transitionTo` * `isActiveForRoute` +Additionally any engines specific code or assertions for url and path generation would be removed, delegating + these responsibilities instead to the router service. For instance, `` should not be aware of `mountPoint`. + #### `RouterService` Provide public import via `@ember/routing/router-service`. +#### Router Helpers + +The changes outlined here improve the DX for engines for the not-yet implemented url helpers specified by [RFC#391](https://emberjs.github.io/rfcs/0391-router-helpers.html). Since those helpers will also utilize the routing service, +they would be compatible with both internal and external engines paths immediately. + ### `ember-engines` package -Provide `router` service that extends from `@ember/routing/router-service`: +While not the scope of this RFC to detail the exact specifics, once these changes are implemented, `ember-engines` +will provide a `router` service that extends from `@ember/routing/router-service`. ```javascript # ember-engines/addon/services/router.js @@ -115,15 +130,14 @@ export default class extends RouterService { } ``` -The extended `RouterService` should override make all public methods and properties play nice with Ember Engines citizen. +The extended `RouterService` would override or extend any necessary public methods and properties to provide a great DX +for engines developers. -> This is the bulk of the RFC. +## Prior Discussions -> Explain the design in enough detail for somebody -familiar with the framework to understand, and for somebody familiar with the -implementation to implement. This should get into specifics and corner-cases, -and include examples of how the feature is used. Any new terminology should be -defined here. +- [emberjs/rfcs#806](https://github.com/emberjs/rfcs/pull/806) +- [ember-engines/ember-engines#587](https://github.com/ember-engines/ember-engines/issues/587) +- [ember-engines/ember-engines#779](https://github.com/ember-engines/ember-engines/issues/779) ## How we teach this @@ -134,28 +148,29 @@ Transition path needs to be published on [ember-engines.com](https://ember-engin 1. Replace use of `` component with `` in every engine codebase. -2. If `ember-engines-router-service` was installed and used, replace any of it' usage with - `RouterService` provided by `ember-engines`. This should be done in the same PR - updating `ember-engines` to the version that ships `RouterService`. +2. Users using `ember-engines-router-service` would need to replace any of it's usage with + the `RouterService` provided by `ember-engines`. This addon was created to solve part of + this problem previously, and would likely be updated to become a true polyfill for the ember-engines + routing service. -> What names and terminology work best for these concepts and why? How is this -idea best presented? As a continuation of existing Ember patterns, or as a -wholly new one? +## Drawbacks -> Would the acceptance of this proposal mean the Ember guides must be -re-organized or altered? Does it change how Ember is taught to new users -at any level? +The removal of assertions within `` specific to ensuring that external paths are properly mapped means that +it is now the responsibility of the routing service provided by ember-engines to enforce that encapsulation is not +broken unintentionally, since a user can now provide any fully qualified path to ``. -> How should this feature be introduced and taught to existing Ember -users? +A similar issue arises with the routing service's `transitionTo` method when invoked with a url. For instance: -## Drawbacks +```ts +router.transitionTo('/'); +``` -> Why should we *not* do this? Please consider the impact on teaching Ember, -on the integration of this feature with other existing and planned features, -on the impact of the API churn on existing apps, etc. +Some prior discussion on this point is [viewable here](https://github.com/ember-engines/ember-engines/issues/587#issuecomment-483531420). +In the case of this RFC, it would similarly become the responsibility of the engine's routing service to prefix such a url with the +engine's mount path as appropriate, or decide to error, to preserve encapsulation. -> There are tradeoffs to choosing any path, please attempt to identify them here. +In practice we feel the flexibility and improved DX of this solution outweighs these drawbacks, and intend to +continue enforcing the same constraints via the engines provided service. ## Alternatives @@ -182,6 +197,12 @@ on the impact of the API churn on existing apps, etc. This option was discussed in [RFC #806 "Engine Router Helpers"](https://github.com/emberjs/rfcs/pull/806) which was considered as partial solution to teh problems discussed in this RFC. + +* Make ember-source handle all of the engine specific logic for routing itself. + + To this point engines have only minimally been accounted for in the code of ember-source, this would digress from that. + This integration could be done at a later time, but it seems unnecessary to currently hoist engines specific logic + into applications that may not have any engines. ## Unresolved questions From 96c318ba113d5caad9e4db2f011e63d872470f75 Mon Sep 17 00:00:00 2001 From: Sergey Astapov Date: Fri, 29 Apr 2022 14:45:58 -0400 Subject: [PATCH 3/4] More tweaks for public RouterService --- ...plate.md => 0000-public-router-service.md} | 57 +++++++++++++++---- 1 file changed, 45 insertions(+), 12 deletions(-) rename text/{0000-template.md => 0000-public-router-service.md} (79%) diff --git a/text/0000-template.md b/text/0000-public-router-service.md similarity index 79% rename from text/0000-template.md rename to text/0000-public-router-service.md index b1c1c1b556..3cf82d5617 100644 --- a/text/0000-template.md +++ b/text/0000-public-router-service.md @@ -34,9 +34,9 @@ Today, users of [`ember-engines`](https://github.com/ember-engines/ember-engines custom components, helpers and services for linking. This leaves the experience feeling sub-par, in addition to introducing extra maintenance friction attempting to keep these primitives aligned with those in `ember-source`. -By providing public a import for `RouterService`, `ember-engines` would be able to provide its own router service -implementation extending from the base implementation. Combined with minor changes to the engines-specfici behavior of -`link-to`, this change would allow us to fill the existing DX gap when it comes to navigating between an Engine +By providing a public import for `RouterService`, `ember-engines` would be able to provide its own router service +implementation extending from the base implementation. Combined with minor changes to the engines-specific behavior of +`link-to`, this change would allow us to fill the existing DX gap when it comes to navigating between an Engine and a host App *and* allow engines to eliminate use of the legacy components required to support 4.x versions of Ember. These proposed changes would further allow `ember-engines` to deprecate the `` @@ -90,6 +90,32 @@ Once this RFC gets implemented, above snippet could be replaced with ### `ember-source` package +This proposal does not seek to make any changes to the default blueprint. +App author may or may not introduce overridden `RouterService` via `app/services/router.js`. + +If app author decide to do so and provide extended `RouterService`, it may look like: + +```js +// app/services/router.js +import RouterService from '@ember/routing/service'; + +export default class extends RouterService { + // ... +} +``` + +In order to make this possible, method `buildRegistry` of `Application` class will be updated to have logic like so: + +```js +import { RouterService } from '@ember/-internals/routing'; + +// ... + +if (!registry.has('service:router')) { + registry.register('service:router', RouterService); +} +``` + #### `` component The use of `-routing` service in `` component would be replaced with the `router` service. @@ -104,26 +130,28 @@ Currently, those methods of `-routing` service used: * `transitionTo` * `isActiveForRoute` -Additionally any engines specific code or assertions for url and path generation would be removed, delegating - these responsibilities instead to the router service. For instance, `` should not be aware of `mountPoint`. +Additionally, any engines specific code or assertions for url and path generation would be removed, delegating +these responsibilities instead to the router service. For instance, `` should not be aware of `mountPoint`. #### `RouterService` -Provide public import via `@ember/routing/router-service`. +Provide public import via `@ember/routing/service`. #### Router Helpers -The changes outlined here improve the DX for engines for the not-yet implemented url helpers specified by [RFC#391](https://emberjs.github.io/rfcs/0391-router-helpers.html). Since those helpers will also utilize the routing service, +The changes outlined here improve the DX for engines for the not-yet implemented url helpers +specified by [RFC#391](https://emberjs.github.io/rfcs/0391-router-helpers.html). +Since those helpers will also utilize the routing service, they would be compatible with both internal and external engines paths immediately. ### `ember-engines` package While not the scope of this RFC to detail the exact specifics, once these changes are implemented, `ember-engines` -will provide a `router` service that extends from `@ember/routing/router-service`. +will provide a `router` service that extends from `@ember/routing/service`. ```javascript -# ember-engines/addon/services/router.js -import RouterService from '@ember/routing/router-service'; +// ember-engines/addon/services/router.js +import RouterService from '@ember/routing/service'; export default class extends RouterService { // ... @@ -148,11 +176,16 @@ Transition path needs to be published on [ember-engines.com](https://ember-engin 1. Replace use of `` component with `` in every engine codebase. -2. Users using `ember-engines-router-service` would need to replace any of it's usage with +2. Users using `ember-engines-router-service` would need to replace any of its usage with the `RouterService` provided by `ember-engines`. This addon was created to solve part of this problem previously, and would likely be updated to become a true polyfill for the ember-engines routing service. +Ember Guides should be updated to rationalize and explain the difference between `app/router.js` and `app/services/router.js`. +The confusion between those two distinct yet different entities exists even today. +However, it's less a problem today as there is only one thing named `router.js` can exist in the app today and +ability to create another such file (even with different purpose and semantics) may be confusing. + ## Drawbacks The removal of assertions within `` specific to ensuring that external paths are properly mapped means that @@ -169,7 +202,7 @@ Some prior discussion on this point is [viewable here](https://github.com/ember- In the case of this RFC, it would similarly become the responsibility of the engine's routing service to prefix such a url with the engine's mount path as appropriate, or decide to error, to preserve encapsulation. -In practice we feel the flexibility and improved DX of this solution outweighs these drawbacks, and intend to +In practice, we feel the flexibility and improved DX of this solution outweighs these drawbacks, and intend to continue enforcing the same constraints via the engines provided service. ## Alternatives From ad03c2bc37236a3ccee0fa21564be6939d87363a Mon Sep 17 00:00:00 2001 From: Sergey Astapov Date: Thu, 12 May 2022 16:01:31 -0400 Subject: [PATCH 4/4] Update PR number --- ...public-router-service.md => 0819-public-router-service.md} | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) rename text/{0000-public-router-service.md => 0819-public-router-service.md} (99%) diff --git a/text/0000-public-router-service.md b/text/0819-public-router-service.md similarity index 99% rename from text/0000-public-router-service.md rename to text/0819-public-router-service.md index 3cf82d5617..26d8e6fb4c 100644 --- a/text/0000-public-router-service.md +++ b/text/0819-public-router-service.md @@ -1,12 +1,12 @@ --- Stage: Accepted -Start Date: 2022-04-21 +Start Date: 2022-05-12 Release Date: Unreleased Release Versions: ember-source: vX.Y.Z ember-data: vX.Y.Z Relevant Team(s): Ember.js -RFC PR: +RFC PR: https://github.com/emberjs/rfcs/pull/819 ---