Make to prop on Link optional again

[sanniassin]

A reimplementation of #3803 for v4

Fixes #2319

[timdorr]

Any reason to have the href={null} prop set? Seems unnecessary.

[sanniassin]

Set to null to avoid mistype when you pass href prop instead of to. Link with non-empty to always generate it's own href, so null is required only for empty one.

[timdorr]

Thanks for the tests! Just need that question above answered and then LGTM.

[pshrmn]

I'm not a fan of this. #5240 brought something about this up as well, and I just don't see the point. A <Link> is meant to provide history powered navigation. If you don't want to use to, just render an anchor.

Edit Maybe that comes off as overly negative. I am open to being convinced that this would be beneficial, but so far I haven't come up with a compelling reason to support this.

[sanniassin]

@pshrmn While it looks like a "hack" of component architecture it's pretty pragmatic for something like a list with disabled elements. Otherwise i have to write a wrapper for Link and i think that empty to is simplier than additional layer of abstraction with wrapper.

import { Link } from 'react-router-dom';

const ListItem = (props) => {
  var { id, availableQuantity, ...rest } = props;
  var outOfStock = !availableQuantity;

  return (
    <Link className="list-item" to={outOfStock ? null : `/item/${id}`>
      {...}
    </Link>
  );
};

Also i can call preventDefault inside onClick but it's confusing to have an non-clickable item with link in browser status bar.

[sanniassin]

What kind of issues can occur if we implement this?

[zemd]

@sanniassin if link is not meant to be matched and active why do you need it? Usually you don't need to render anchor at all if , for example, your products are out of stock.

{outOfStock ? (<span className="link">Out of stock</span>) : (<Link to="/item">Item</Link>)}

[sanniassin]

@zemd It doesn't scale well for more complex components.

<Link to={outOfStock ? null : `/item/${item.id}`} className="item">
  <img className="item__picture" src={item.imageUrl} />
  <div className="item__name">{ item.name }</div>
  <div className="item__description">{ item.description }</div>
  <div className="item__tags">
    { item.tags.map((tag, i) =>
      <div className="item__tags-item" key={i}>{ tag }</div>
    )}
  </div>
</Link>

vs

outOfStock ? (
  <div className="item">
    <img className="item__picture" src={item.imageUrl} />
    <div className="item__name">{ item.name }</div>
    <div className="item__description">{ item.description }</div>
    <div className="item__tags">
      { item.tags.map((tag, i) =>
        <div className="item__tags-item" key={i}>{ tag }</div>
      )}
    </div>
  </div>
) : (
  <Link to={`/item/${item.id}`} className="item">
    <img className="item__picture" src={item.imageUrl} />
    <div className="item__name">{ item.name }</div>
    <div className="item__description">{ item.description }</div>
    <div className="item__tags">
      { item.tags.map((tag, i) =>
        <div className="item__tags-item" key={i}>{ tag }</div>
      )}
    </div>
  </Link>
)

[zemd]

@sanniassin it scales indeed as it should scale. if you have some common pieces of code you can always utilize them.

[sanniassin]

@zemd I guess it depends on a preferred code style. Link is a replacement for an anchor tag and anchor can have no href, so it sounds intuitive for me to expect the same behaviour from Link.

Empty to is totally safe. It allows you to write less code without maintenance and readability trade-offs (i think there is no ambiguity in what to={outOfStock ? null : `/item/${item.id}`} means). And it doesn't affect you if you prefer to always specify to.

[sanniassin]

An argue to implement in related issue: #2319 (comment)

Any decisions?

[vladshcherbin]

@sanniassin is this not working for you?

outOfStock ? (
  <ProductItem item={item} />
) : (
  <Link to={`/item/${item.id}`} className="item">
    <ProductItem item={item} />
  </Link>
)

[sanniassin]

@vladshcherbin It works, but it means that it's a ProductItemContainer not a ProductItem itself, so you have one more component and one more file in your project. You may also have conflict in styles when Link and ProductItem share the same className. I found solution with empty to much simpler and cleaner.

[mjackson]

Thanks for the PR @sanniassin, but I think I'd suggest you do as @vladshcherbin (and others) suggest. If you don't need <Link to>, you shouldn't be rendering a <Link> at all.

[sanniassin]

@mjackson Should we add a note to migration guide then? Because v3 supports empty to.

[mjackson]

@sanniassin Yes. I wasn't even aware we had a migration guide! (That's awesome that we do though!)

[silouanwright]

@zemd I disagree with your assessment. Take a look my comment here for further elaboration: #2319 (comment)

To sum it up (again), <Link /> is an interface for an anchor. There is a reason the HTML spec does not require a tags to have href's on them, they could have forced you to always use an href... they recognized you want something to appear as a link (and be a link as far as the DOM is concerned) if you don't yet have the path. If I have a component that has a bunch of links, but I don't have the paths yet (let's say I'm getting that data from redux or from some data store that doesn't necessarily give me the data the moment the component mounts and is visible). It would be a bad user experience to not render them as links, especially if the css of links is vastly different from that of a non link (let's say anchors have more font size or something that makes their dimensions bigger than regular text). That would be extremely bad UX having text change dimension. For that and other reasons, you wouldn't (and shouldn't) do the 2nd part of @sanniassin 's example or @vladshcherbin 's example in almost in any scenario. That's the response to the first half of your argument. Now for the 2nd half.

Now technically sure, the user can wrap Link on their own. This is what I do in my repo if my memory is correct:

const CustomLink = props => {
  const buttonElement = props.to ? Link : 'a';
  return <ButtonElement {...props} />
}

That being said, the thing to remember, is <Link /> is a convenience method. Someone not having an href (or path) yet because they need to request it, but they'd like it to still render as an anchor... to me is a reasonable (and convenient) request given the nature of Link anyway, as you could easily route the user without Link to begin with. And Link is an abstraction over an anchor anyway. So I think it makes sense that the prop be optional as hrefs (that are derived from ) are optional in the HTML spec anyway. Why go against the grain (unless there are performance reasons)?

Now that being said, I'm not really petitioning for the feature anymore... I'm using a different routing libraries. I just wanted to explain my reasoning, as I made an argument that @sanniassin actually posted in this thread and neither him nor I ever received any response to it, so I was sort of confused coming back here... we continued the conversation it seems like, without a response to the points I made 😄

[timdorr]

@reywright No, <Link> is critical to using the Router. It is not just a wrapper or convenience method. A normal <a> will issue browser navigation events. Only <Link> will connect to the router and use the local history to navigate without loading up a new page.

[silouanwright]

@timdorr how so? You can very easily have an click handler for the onClick of an anchor, and inside the handler, e.preventDefault, then use history.push(). In fact if you're working within redux, it makes more sense not to use <Link / > in a lot of cases as you want to fire off an action to load your data first, then route the user once you have the data. (you'd use push from react-redux which is an action creator for history.push() if you're using redux). I did this exact thing in production for quite sometime before ever touching <Link />. Unless this changed in the newer version of the Router for whatever reason, it's not absolutely necessary to use it.

It is quite convenient though.