Router link component and routing helpers

[NullVoxPopuli]

my first RFC!

rendered

[rwjblue]

FWIW, this is what ember-router-helpers was aiming to implement (though I did a very poor job on the README so who would ever know 😝 ).

[rwjblue]

Thanks for putting the RFC together! I personally think that we should separate adding named arguments to link-to from adding the other router based helpers.

Also, the Router Service RFC specifically mentions a number of the helpers you have proposed...

[rwjblue]

I believe that we can be more iterative here, and add the following named arguments to the existing link-to:

• route (the route to link to)
• models (an array of model / primitive ids / etc)

This would make transforming existing {{link-to}} usages very easy, and still largely satisfy (what I thought) was the main concern: desire to use the newer angle bracket invocation style.

[NullVoxPopuli]

desire to use the newer angle bracket invocation style was my original concern. but I couldn't come with up with named argument for link-to that didn't feel awkward, or implicitly imply functionality that might not be there, such as would be the case with {{link-to route=''}} (what else could we link to?)

[rwjblue]

Ha! I was actually thinking exactly the opposite. <Link seems like it should manage all links, not just ones within the routing structure.

Also, I think there is value in iterating forward within the component we all know / love / hate / etc. Adding a couple of named arguments to {{link-to}} seems like it would unlock using angle bracket invocation and be a fairly small change/tweak to the internal code.

[NullVoxPopuli]

lol, guess the only way forward is with the smallest amount of changes. I'm totes good with named args in linked to. :)
less change = less potential for confusion

[rtablada]

I would be 👍for making LinkTo work with named attrs and even named models.

I actually looked at this when I was first playing with components.

With a router.js (ignore that the route inheritance wouldn't really make sense

this.route('profile', { path: '/:userId' }, function() {
	this.route('friend', { path: '/friend/:friendId' })
})

My ideal LinkTo invocation would be

<LinkTo @routeName="profile.friend" @models={{hash userId=model.user.id friendId=friend.id}}>Some Test</LinkTo>

I think this could actually be done completely backwards compatible while allowing positional params too (with deprecation later).

[NullVoxPopuli]

ooo, @rtablada I like that

[rtablada]

The names are a bit different and I would need to ask someone a bit more knowledgeable with the router to make named @model hash work but here's a proof of concept of the changes needed to make params or named attributes work:

rtablada/link-to-params@5236ac5

The links I tested this with are:

<LinkTo @params={{array "foo" 1 2}}>Positional Params</LinkTo>

<LinkTo @targetRouteName="foo" @models={{array 1 2}}>Named Params</LinkTo>

[NullVoxPopuli]

hmm, we'd probably want to alias targetRouteName to something shorter for the public api, yeah?
I'm glad what you have is so simple though!

[rtablada]

I think naming it RouterLink would make it clearer that this is router coupled.

[gossi]

I would rather see <Link @to='route.name'>...</Link> for simple use cases that don't need params. For those that need them: <Link @to='route.name' @params=(array ....)>...</Link> that might be an easier to grasp API, especially for beginners.

[NullVoxPopuli]

So, how about this:

Phase 1:

• add named arguments to LinkTo: @routeName and @models

Phase 2:

• add Link component with @to and @params
• deprecate LinkTo

Question: RouterLink vs Link? (personally, I favor Link, though, because components aren't imported like they are in react, maybe having the context of RouterLink or Router::Link (or however namespaces or going to work) would make sense, too? idk)

Phase 3:

• remove LinkTo and deprecation

[sandstrom]

@NullVoxPopuli I haven't read the full RFC yet (I will).

One issue that immediately comes to mind is this discussion: #275

This addon is related to the aforementioned RFC:
https://github.com/alexspeller/ember-cli-active-link-wrapper

(perhaps the content of those two links can be helpful in some what, when iterating on this proposal)

[rtablada]

@NullVoxPopuli @params might leak internal component implementation and I don’t think we could use that attribute name for a long time as Ember component uses @params for positional params.

Maybe I’m missing something but after adding named args, why would we need to make a completely different component with different attribute names? Seems counter intuitive? At best renaming to the Router namespace.

[chrism]

I really like idea of a Link component with named arguments and deprecating the current link-to.

My comment is more of an open question—could we use this as an opportunity to also address the beginner bear trap of passing a model versus an id ie whether or not the model hook gets fired on the route?

I feel like this might be a good moment to see if there is a way to reduce the potential confusion, though I don't feel confident proposing a solution myself.

[NullVoxPopuli]

@chrism

My comment is more of an open question—could we use this as an opportunity to also address the beginner bear trap of passing a model versus an id ie whether or not the model hook gets fired on the route?

I think if the params/model argument has a hash, it could help? having only positional args really confused me when I was first learning ember.

I think being able to specify id or model by convention might be the way to go:

<Link @to='posts.edit' @params={{hash user=userModel postId=4}}>
  Edit {{userModel.name}}'s 4th post
</Link>

I know @rtablada said @params might leak internal component implementation, so if we want this API, we'll need to be very careful about that, or maybe make Link a Glimmer Component? idk

[chrism]

I think if the params/model argument has a hash, it could help? having only positional args really confused me when I was first learning ember.

Definitely. I think that using the convention of modelname vs modelId to highlight the distinction is a great idea—especially if the documentation makes it clear exactly what the difference means in practice (ie whether or not to use the model hook).

Whether or not that would completely resolve the issue I don't know, but my feeling is that it would be a big help in clarifying the two different use cases.

[btecu]

I wonder if there's anything we can borrow from ember-href-to.
cc @GavinJoyce

[luxzeitlos]

If we can do

onclick={{action (transition to='posts.edit' postId=post.id)}}

why can't we do

<Link @to='posts.edit' @postId=post.id />

the Link component becomes a bit more complex, but in my opinion it would be much cleaner because it makes refactoring from a link to an action much easier.

[gossi]

If that works out, since @postId is some kind of a wildcard argument to the component (couldn't be defined at creating the component, since it is unknown to it). If it does, I'm happy in favor of it.

[luxzeitlos]

@gossi first this is ember internal so it could use private API.

But I think its even possible with public API. Object.keys(this.attrs)/Object.keys(this.args) inside didReceiveAttrs should do the trick:

    didReceiveAttrs() {
      this.set('to', Object.keys(this.attrs).reduce((to, key) => {
         to[key] = this.attrs[key]
         return to;
      }, {}));
    }

However then we need a way to pass it to the transition and is-route-active helpers. Because we don't have a ... operator in handlebars yet we can't do {{action (transition ...this.to)}}, but essentially thats what we want. So maybe `transition could accept a special named argument which could be used for this:

    {{transition to=@to params=this.to}}

while this could be useful as public API it could kept private and only used by the <Link> component.

---

could it maybe also be a good idea to expose a href-to helper? because the current proposed <Link> component misses a href, and a custom href could be useful for a custom version of the <Link> component. Essentially this is ember-href-to.

---

And one last thing, maybe it should be specified what should happen if the user does <Link to='posts' onclick={{action 'foo}}>.

[locks]

Some higher-level thoughts!

[locks]

You went from an H2 to an H4 here.
I would suggest dropping this title, however, and writing a small intro explaining your rationale regarding splitting the work into phases, and why each specific phase.

[locks]

Positional arguments are not planned for angle bracket invocations, I suggest updating this sentence to remove any "but in the future it might" implicit suggestion.

[locks]

This feels more of an open question than a detailed design, due to the uncertainty of the proposal.

[locks]

Typo in "becaus"

[locks]

I'm thinking we should skip including this, because that is the alternative for every RFC, and the body of the RFC is an active argumentation against doing nothing.

[courajs]

Why bother adding the named params to link-to, if we're immediately afterwards going to add a new component to supersede it? It seems like unnecessary work - just introduce the new component and deprecate the old one.

[NullVoxPopuli]

Why bother adding the named params to link-to, if we're immediately afterwards going to add a new component to supersede it? It seems like unnecessary work - just introduce the new component and deprecate the old one.

@courajs I'd be a fan of this. If other people are, I'll remove those relevant parts of the RFC. :)
(and then also address @locks' comments)

[NullVoxPopuli]

Updated. Also started some work over here: adopted-ember-addons/ember-router-helpers#46

[buschtoens]

Things that seem to be missing:

• href
• replace=true

[NullVoxPopuli]

@buschtoens thanks for bringing those up! Those lend themselves nicely to named arguments :)

[gossi]

that doesn't make sense anymore

[NullVoxPopuli]

I edited too quickly. lol

[gossi]

I saw, you changed some of the is-route-active helpers to is-active (not all, you missed one :p). Though the is-active is very generic and has a potential for conflicts (also MU works on {{use}}), this should be precise like the is-route-active before.

[NullVoxPopuli]

@gossi I think that makes sense -- I'll revert to is-route-active :)

[NullVoxPopuli]

@buschtoens replace is mentioned in the transition helper.
and href would just be the anchor attribute yeah?

(I'll add ...attributes to the example implementation of Link)

[NullVoxPopuli]

@buschtoens, @gossi: updated

[luxzeitlos]

Just to clarify any misunderstanding:

I think we also need a href helper. Something like (href to='item' item='12'). This is needed for the Link component. The Link component should always have a correct href attribute. This is currently true for the link-to component.

[NullVoxPopuli]

thanks @luxferresum I'll get that added! :)
and also add it to ember-router-helpers if it isn't in there already.

The goal of this RFC is to have equivalent capabilities from {{link-to}}, but with any custom implementation.

[buschtoens]

I made ember-link which gives you a renderless version of <LinkTo> and complies with the API proposed in RFC 459 "Angle Bracket Invocations For Built-in Components", but is reasonably close to this PR.

<Link
  @route="some.route"
  @models={{array 123}}
  @query={{hash foo="bar"}}
as |l|>
  <a
    href={{l.href}}
    class={{if l.isActive "is-active"}}
    {{on "click" l.transitionTo}}
  >
    Click me
  </a>
</Link>

[jrjohnson]

This is an accessibility red flag to me, links should be links. While the transition helper probably has value in other places I'd like to see an example that doesn't just turn a button into a styled link.

[NullVoxPopuli]

Superseded by (at the time of writing) to-be-implemented functionality described by https://github.com/emberjs/rfcs/blob/master/text/0391-router-helpers.md#event-dispatcher which would be much more ergonomic