Public router service #819

SergeAstapov · 2022-05-12T20:00:08Z

chriskrycho · 2022-05-12T21:21:23Z

Thanks for writing this up! One question I have: is the idea that the router subclass could override all parent methods? Or should some of them be private (in TS terms and also in actual TS implementation)?

SergeAstapov · 2022-05-12T21:27:12Z

@chriskrycho as currently only methods listed as part of public API are actually implemented in RouterService, I assume we should keep all the methods as public and overridable as ember-engines would need to do those overrides.

runspired · 2022-05-12T21:33:41Z

is the idea that the router subclass could override all parent methods? Or should some of them be private (in TS terms and also in actual TS implementation)?

Confirm what @SergeAstapov said, currently there is no private API in the router service, and Ideally it stays that way (basically as more of an interface).

This has the practical effect that at least (for now) overridden methods should eventually call super in order to have actual routing things take effect, minimizing the potential for folks to get too ---creative--- with what they might do as the signature and intent will have to remain aligned.

I considered whether we should call this out in the RFC and opted against, mostly because it seemed to fall under "public APIs are public" and telling someone they must call super and must extend the base-class would artificially (and only pedantically) limit experimentation in this layer that is desired.

villander

I really loved this RFC, thanks for doing that @SergeAstapov, but I think we need to keep the readability and isolation principle on top of our head to keep moving on in a direction to provide compatibility without losing the ergonomy 😄

villander · 2022-05-12T22:24:59Z

text/0819-public-router-service.md

+* links to external routes in templates within the engine rendered via `<LinkTo />` component:
+
+  ```hbs
+  <LinkTo @route="external-route">Go Somewhere</LinkTo>


I'm still thinking that we should use <LinkTo @route="external.home">Go Somewhere</LinkTo> where external (hypothetical name) means a prefix that it's an external route. It's good for ergonomics because as a developer we don't want to go to externalRoutes definition and check if it's a internal or external route. I see the importance of deprecating <LinkToExternal/> but I don't want to lose the readability that it means an external link without poking around the code.

Also it solve the drawback on this RFC 😄

@villander this would prevent addons from utilizing LinkTo, which I think makes it a non-starter. It might be interesting to bring the external-routes map into routes.js at some point to address this though so that there's only one location where valid routes may be defined. This would be outside the scope of this RFC.

Looking for this perspective I agree with you @runspired I think we just need to find some way to don't make engines more confusing and harder to teach than it already is.

villander · 2022-05-12T22:27:52Z

text/0819-public-router-service.md

+  This is not viable long term solution as any changes made to `<LinkTo />` component in the Ember.js codebase
+  would need to be backported to `ember-engines`.
+
+* Deprecate and remove `externalRoutes` mapping.


🚨 It breaks the isolation principle around ember-engines, where every engine could access any outside contexts without asking for it, going in the opposite direction of code isolation proposed by Engines.

right, that's why though this is an alternative it's not the suggested path of the RFC

Surely, just wanted to highlight and provide a reminder/support to the next reviews 😃

villander · 2022-05-12T22:29:26Z

text/0819-public-router-service.md

+
+1. Replace use of `<LinkToExternal @route="external-route" />` component
+   with `<LinkTo @route="external-route" />` in every engine codebase.
+2. Users using `ember-engines-router-service` would need to replace any of its usage with


Suggested change

2. Users using `ember-engines-router-service` would need to replace any of its usage with

2. Users using [ember-engines-router-service](https://github.com/villander/ember-engines-router-service) would need to replace any of its usage with

villander · 2022-05-12T22:32:59Z

text/0819-public-router-service.md

+  export default class extends Component {
+    @service router;
+    @action clickLink () {
+      this.router.transitionTo('external-route');


Also, I'd like to have something like that to don't lose the explicit outside context on engines. @rwjblue @dgeb and I made huge progress on that in the past - ember-engines/ember-engines#779

import { external } from 'ember-engines/helper' ... @action clickLink () { this.router.transitionTo(external('external-route')); }

It feels unnecessary to impose an extra function call on users to mark something as external when it has already been defined as external in their router configuration (see also comments above on external.). Similarly, this would prevent addons from making use of the router service cleanly.

It depends, I kinda agree with you that mapping those things on the router configuration clarifies things on a certain level, but I love readable and explicit code. How would you realize just looking for a ton of transitionTo in your code that those routes are from the host app? imagine you have at least 50 occurrences how many are linking outside context? Correct me if I'm wrong, IMO is necessary back in forth on the router configuration to realize those things.

chriskrycho · 2022-05-13T03:41:14Z

Re: "no private API"—I want to be clear here about two things:

I don’t have a vested interest in the particular decision we make here! I just want to make sure we make it carefully and with clear understanding of what we're committing to. 😄
Directly related: the fact that everything has been public so far has been uninteresting just insofar as it has not been subclass-able and therefore not overridable. Supporting subclasses overriding methods has very different semantic and maintenance implications than public call-ability. (This comes up in any kind of formal analysis of types, whether dynamic or static; ask me how I know. 😂) In particular, public call-ability does not have the same implications for upholding internal-only invariants that subclass-ability does.

Net, I think that it's worth being very explicit and intentional about this question and I would actually like to see it addressed explicitly in the RFC. I'd also like to see some actual analysis of the existing public API: do we actually want to say that it's fine for subclasses to override the definition of currentURL, for example? Or of location, or the timing and behavior of firing routeWillChange and routeDidChange events? Maybe, but that's not obvious to me!

Accordingly, I think it would be helpful to delineate, for each of these (perhaps at the category level if appropriate, but at a per-item level if that is more appropriate in other cases) whether it should be overridable by subclasses or not—

Methods:

Properties:

Events:

Bonus doh comment: it can’t actually use private in TS for the implementation details, because it is public for call-ability. But per both general good norms and per the SemVer for TS Types spec, it’s perfectly fine for us to define what the public API supports in terms of subclassing!

chancancode · 2022-05-13T19:12:16Z

(FYI we didn't get to this agenda topic this week because #811 and #812 took up most of the time, will try again next week)

chancancode · 2022-05-13T19:19:19Z

I don't have a ton of context on this, but maybe this comment from @ef4 will be important/useful to y'all since you have been thinking about it?

NullVoxPopuli · 2022-05-14T01:44:02Z

text/0819-public-router-service.md

+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


this is also solved by using hrefs instead of route/model/query args of the default link-to.
with an a tag with preventDefaultd click handler, we can use the router service to detect when an href is in the route, or in an external app, and appropriately "do the right thing.

I like this approach because it means we:

don't need extra components for each type of usage

uses the existing href knowledge that everyone already understands with <a>

aligns with: (the spirit of): Implement in-app transitions for native anchor elements so that LinkTo isn't needed ember.js#19271 and Router Helpers #391

here is a demo: https://codesandbox.io/s/custom-link-component-dgbxl?file=/app/components/link.hbs
(this demo is incomplete, but gives the gist).
(except codesandbox doesn't work so... 🤷 (this sandbox has been untouched since 3.21, and the dependency checker is preventing start up... idk what's going on. ))

where improvements could be made here where <LinkTo> is a bit better at:

query-param only transitions

sticky query params

though, unifying on LinkTo is still a good improvement!
(over multiple components)

This RFC is what would make those router helpers and the href approach work correctly for engines as well.

wagenet · 2022-07-25T14:10:10Z

It looks like this has some interest so we should work to keep it moving.

villander · 2023-08-07T14:17:46Z

Can we move this forward?

@runspired @chriskrycho @wagenet @chancancode

achambers · 2023-09-01T14:39:32Z

Can we move this to Exploring?

wagenet · 2023-09-22T19:05:07Z

We had some back and forth in the Review meeting today around whether this should move to Exploring or not. There's definitely agreement that this is a problem we need to resolve, but that it's probably not going to be something that we can resolve until the new router design is more finalized.

SergeAstapov and others added 4 commits April 21, 2022 14:49

Public RouterService

260d449

Weekly Meeting Review Changes

3d84a3f

More tweaks for public RouterService

96c318b

Update PR number

ad03c2b

rwjblue self-assigned this May 12, 2022

rwjblue added T-framework RFCs that impact the ember.js library T-ember-engines RFCs that impact the ember-engines library labels May 12, 2022

villander reviewed May 12, 2022

View reviewed changes

This was referenced May 13, 2022

Bring the router service to engines ember-engines/ember-engines#587

Open

Merge internal and external route namespace, drop *External methods & <LinkToExternal> ember-engines/ember-engines#691

Open

NullVoxPopuli reviewed May 14, 2022

View reviewed changes

SergeAstapov mentioned this pull request May 24, 2022

[WIP] [BUGFIX] Make RouterService.transitionTo engine aware emberjs/ember.js#18569

Closed

villander mentioned this pull request Jul 25, 2022

Align the mount and route APIs #722

Closed

This was referenced Aug 3, 2022

feat: add isExternal argument to simplify ember-engines emberjs/ember.js#20151

Closed

Refactor LinkToExternal to use LinkTo directly ember-engines/ember-engines#810

Closed

wagenet added the S-Proposed In the Proposed Stage label Dec 2, 2022

SergeAstapov mentioned this pull request Aug 23, 2023

Ember >= 4.x compatibility (require ember-source 4.8+) emberjs/ember-legacy-built-in-components#22

Merged

wagenet added S-Exploring In the Exploring RFC Stage S-Proposed In the Proposed Stage and removed S-Proposed In the Proposed Stage S-Exploring In the Exploring RFC Stage labels Sep 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Public router service #819

Public router service #819

SergeAstapov commented May 12, 2022 •

edited

Loading

chriskrycho commented May 12, 2022

SergeAstapov commented May 12, 2022

runspired commented May 12, 2022 •

edited

Loading

villander left a comment •

edited

Loading

villander May 12, 2022

runspired May 12, 2022

villander May 13, 2022

villander May 12, 2022 •

edited

Loading

runspired May 12, 2022

villander May 13, 2022

villander May 12, 2022

villander May 12, 2022 •

edited

Loading

runspired May 12, 2022 •

edited

Loading

villander May 13, 2022 •

edited

Loading

chriskrycho commented May 13, 2022

chancancode commented May 13, 2022

chancancode commented May 13, 2022 •

edited

Loading

NullVoxPopuli May 14, 2022

NullVoxPopuli May 14, 2022 •

edited

Loading

runspired May 14, 2022

wagenet commented Jul 25, 2022

villander commented Aug 7, 2023

achambers commented Sep 1, 2023

wagenet commented Sep 22, 2023

	2. Users using `ember-engines-router-service` would need to replace any of its usage with
	2. Users using [ember-engines-router-service](https://github.com/villander/ember-engines-router-service) would need to replace any of its usage with

Public router service #819

Are you sure you want to change the base?

Public router service #819

Conversation

SergeAstapov commented May 12, 2022 • edited Loading

chriskrycho commented May 12, 2022

SergeAstapov commented May 12, 2022

runspired commented May 12, 2022 • edited Loading

villander left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

villander May 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

villander May 12, 2022 • edited Loading

Choose a reason for hiding this comment

runspired May 12, 2022 • edited Loading

Choose a reason for hiding this comment

villander May 13, 2022 • edited Loading

Choose a reason for hiding this comment

chriskrycho commented May 13, 2022

chancancode commented May 13, 2022

chancancode commented May 13, 2022 • edited Loading

Choose a reason for hiding this comment

NullVoxPopuli May 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wagenet commented Jul 25, 2022

villander commented Aug 7, 2023

achambers commented Sep 1, 2023

wagenet commented Sep 22, 2023

SergeAstapov commented May 12, 2022 •

edited

Loading

runspired commented May 12, 2022 •

edited

Loading

villander left a comment •

edited

Loading

villander May 12, 2022 •

edited

Loading

villander May 12, 2022 •

edited

Loading

runspired May 12, 2022 •

edited

Loading

villander May 13, 2022 •

edited

Loading

chancancode commented May 13, 2022 •

edited

Loading

NullVoxPopuli May 14, 2022 •

edited

Loading