eslint-plugin-jsx-a11y icon indicating copy to clipboard operation
eslint-plugin-jsx-a11y copied to clipboard

Add option for passed hrefs (as in Next.js) (anchor-is-valid)

Open sk22 opened this issue 6 years ago • 31 comments

I'd suggest adding an option for a tags that don't have an href because its containing component passes the href to it.

For example, Next.js' Link component takes the href but requires an a tag as a child:

<Link href="/about">
  <a>About</a>
</Link>

Creating such structure triggers the anchor-is-valid rule:

[eslint] The href attribute is required on an anchor. Provide a valid, navigable address as the href value. (jsx-a11y/anchor-is-valid)

At the moment, I don't see any practical usage of the anchor-is-valid rule that allows passing the href to the child.

A possible fix could be to check if the parent component (i.e. Link) has the passHref prop set (which is optional in Next.js when the child is an a tag without an own href). Other than that, the rule could just check for the <Link href><a></Link> structure.

sk22 avatar Feb 25 '18 11:02 sk22

Here's a workaround (i.e., disabling the rule's noHref aspect):

"jsx-a11y/anchor-is-valid": [ "error", {
  "components": [ "Link" ],
  "specialLink": [ "hrefLeft", "hrefRight" ],
  "aspects": [ "invalidHref", "preferButton" ]
}]

sk22 avatar Feb 25 '18 12:02 sk22

That sounds like a really strange API; if it’s a Link why would it need an a tag to be passed in? All that does is force a cloneElement, needlessly

ljharb avatar Feb 25 '18 15:02 ljharb

It's indeed a strange API. The reason they did it like this is probably so the Link element can be rendered as any, not only an anchor. But there's an as prop on Link, I'll just check out if this can be used instead. Edit: Nevermind as; seems like it does something completely else.

sk22 avatar Feb 25 '18 16:02 sk22

Btw; yep, what you said is literally what the Next.js Link does... https://github.com/zeit/next.js/blob/canary/lib/link.js#L149 Also see this commit: https://github.com/zeit/next.js/commit/6431f5fce2593d8cadb81841eb0e717facbe4aa6

sk22 avatar Feb 25 '18 16:02 sk22

I’m not sure why they wouldn’t accept a component (or string “a”) so they can render the element directly rather than cloning :-/

Either way I’m not sure that supporting this generically is something either possible or reasonable to do.

ljharb avatar Feb 25 '18 16:02 ljharb

I see... Thanks!

sk22 avatar Feb 26 '18 07:02 sk22

I’ll leave this open if somebody has an idea of how to handle it, but hardcoding an arbitrary library’s strange convention is a nonstarter.

ljharb avatar Feb 26 '18 16:02 ljharb

I'm not sure how to handle this but I'm also running into the same issue:

image

trevordmiller avatar Jul 20 '18 15:07 trevordmiller

Yeah, this is really a fault of the next.js Link API. I submitted an issue. https://github.com/zeit/next.js/issues/5533

danny-andrews-snap avatar Oct 26 '18 17:10 danny-andrews-snap

Here's a workaround (i.e., disabling the rule's noHref aspect):

"jsx-a11y/anchor-is-valid": [ "error", {
  "components": [ "Link" ],
  "specialLink": [ "hrefLeft", "hrefRight" ],
  "aspects": [ "invalidHref", "preferButton" ]
}]

This will disable all linting a tags without href, is there a way to:

<Link href='/'><a>Disable lint error</a></Link>
<a>Enable lint error</a>

T04435 avatar Jul 30 '20 00:07 T04435

@T04435 looking for the same solution. Disabling eslint for the nested anchor inside the Link, and not globally.

takebo avatar Aug 06 '20 12:08 takebo

In response to @sk22 adding the following to .eslintrc.json still causes errors.

"invalidHref", "preferButton"

Does this also mean, eslint is suggesting to use <button> instead of <a> tag right? For Next.js will replace a button work within <Link>?

I would rather turn off the rule to make it work with Next.js.

trigunam avatar Oct 10 '20 06:10 trigunam

If it has a URL to navigate to, it should be an a, which in next.js and react-router means a Link. A button should only be for non-navigation behavior.

ljharb avatar Oct 10 '20 06:10 ljharb

If it has a URL to navigate to, it should be an a, which in next.js and react-router means a Link. A button should only be for non-navigation behavior.

But why not? Button can be used to redirect user to some page. For example the form usually has a "submit" button and "cancel" - and end user by pressing cancel button is waiting to be redirected out of the form and "submit" button also should submit and redirect somewhere. It's a natural behavior.

FDiskas avatar Oct 21 '20 20:10 FDiskas

@FDiskas because that’s not how semantic html works. Buttons are for submitting forms, and that’s not navigation. Cancel buttons shouldn’t exist; that should either be a link, or the normal browsers’ reload button.

ljharb avatar Nov 03 '20 03:11 ljharb

The suggested linting workaround to disable the noHref check works, but there are additional checks done on <a>s that can give more errors. For example, adding an onClick={} to an <a> triggers multiple rules incorrectly, including "Static HTML elements with event handlers require a role" (no-static-element-interactions), because anchors "without" hrefs are static elements.

While this is obviously a weirdness with Next.js' Link component inserting the href into the <a> after jsx-a11y is run, a potential solution could be some sort of config to disable all rules for children of particular components that jsx-a11y doesn't understand?

burntcustard avatar Dec 21 '20 19:12 burntcustard

An alternative (but hacky) solution is to enable the passHref property and set the inner href to a dummy value. Next.js will override the dummy value with the correct value from the outer component, so everything will work correctly and it'll pass the jsx-a11y/anchor-is-valid rule.

  <Link href="/my-amazing-page" passHref>
    <a href="dummy">Go to my amazing page</a>
  </Link>

MartinDevillers avatar Jan 14 '21 14:01 MartinDevillers

I just put my hacky solution up as a gist - I've been using a wrapper component that internally creates the <Link> and <a> components together. This way you can add the LinkTo component to this rule's components list and it will validate properly.

zackdotcomputer avatar Jan 19 '21 14:01 zackdotcomputer

As usually you won't use the Link component on many pages (mostly only within a Navigation or Layout component), you could also easily disable the next line:

return (
    <Link href={yourUrl}>
        {/* eslint-disable-next-line jsx-a11y/anchor-is-valid */}
        <a {...yourProps}>{yourLinkName}</a>
     </Link>
);

wheelmaker24 avatar Feb 01 '21 18:02 wheelmaker24

@wheelmaker24 Agreed, a one-by-one disable or the passHref solution above both work fine if your site only needs a few Links and/or they're gathered into a couple central components.

I don't think those are going to be efficient if you're building a site that uses links in more contexts, though, like an ecommerce or a webapp. If you have Links all over the place you're basically stuck either turning off the rule in your config or pulling in a wrapper component to work around the issue until Next or ESLint can fix this. :-\

zackdotcomputer avatar Feb 01 '21 21:02 zackdotcomputer

There's nothing we can really do except hardcoding the next.js pattern, which is all kinds of brittle. The proper solution is for next.js to follow the react best practice that long predates their existence: avoid patterns that necessitate cloning elements.

ljharb avatar Feb 01 '21 21:02 ljharb

No disagreement on what the ideal solution is @ljharb, but I disagree that hardcoding an exception for the Next.js pattern is the only non-brittle solution. If we could not only add certain components to the scope of this rule (as the "components" argument allows us to) but also to exclude <a> components (which we are specifically not allowed to do currently), then we could configure this rule to work with the Next.js pattern on our side as the end-user without needing to make any hardcoding to this project.

zackdotcomputer avatar Feb 01 '21 22:02 zackdotcomputer

@zackdotcomputer true. but then users would be able to exclude <a> components when not using this specific Next.js pattern, and I don't think that downside is worth it.

ljharb avatar Feb 01 '21 22:02 ljharb

If you're worried about missing un-nested <a> tags in a Next.js project, I know we could find a way to make the configuration sufficiently targeted (e.g. make it specifically "ignore <a> tags that are direct children of these tags...").

If you're just worried about developers being able to disable <a> validation in a more general way, I think it'd still be a net upside if a flag like that could rescue developers who would otherwise disable <a> validation by just disabling this rule entirely. Even as a developer who cares enough about a11y and linting to be here in this thread, if a rule is fighting my tech stack and I can't configure them to play nicely, I'm going to turn off the rule before I swap out the tech stack.

zackdotcomputer avatar Feb 02 '21 00:02 zackdotcomputer

Hmm. I suppose if:

  • Link is in the components setting
  • the <a> is missing an href, but is directly contained as the only child in an otherwise-valid link component
  • a new option is provided to enable this behavior

then perhaps the risk is minimal, and the only downside would be that we'd have to maintain a rule option forever because a single framework made a poor design choice and hasn't fixed it after a number of years :-/

ljharb avatar Feb 02 '21 18:02 ljharb

¯\_(ツ)_/¯ yup, that's about the speed of things. Just eslint's bad luck as a build-tool that the framework that made that bad choice also happened to become super popular.

Since I'm having trouble reading the tone of voice on your post as to whether you will or won't consider adding this option, I'm going to open a PR to add a "Case: I use Next.js Link Components" to the description of this rule so that folks can more easily find this thread of workarounds. If this option winds up getting added, that case can be updated to point users to it.

zackdotcomputer avatar Feb 03 '21 10:02 zackdotcomputer

This is my work around

// MyLink.tsx
import React, { PropsWithChildren } from 'react';
import Link, { LinkProps } from 'next/link';

// This will allow you to pass any other props that the next Link supports
export type MyLinkProps = LinkProps;

const MyLink = (props: PropsWithChildren<MyLinkProps>) => {
  const { children, ...linkProps } = props;
  return <Link {...linkProps} passHref ><a href="passRef">{children}</a></Link>;
};

export default MyLink;


# usage

<MyLink href="/about" scroll >About</MyLink>

T04435 avatar Feb 13 '21 05:02 T04435

@zackdotcomputer your tone read is accurate, as I'm not convinced either way myself, as every outcome seems differently terrible. I'll take a look at the PR.

ljharb avatar Feb 16 '21 17:02 ljharb

I know this problem from nextjs, but just started using @ui-router/react for an hybrid angularjs application (I know, ugh), and it has the same api.

chiptus avatar Sep 26 '21 10:09 chiptus

@chiptus I noticed that the new NextJS eslint env turns off this rule since it can't be configured to work with their API. I think the unfortunate end state here for at least the near future is that this rule will have to be disabled if one is using a framework that follows the "wrapper component takes the href" pattern.

That pattern doesn't seem to be going away and there doesn't appear to be any movement to alter this rule to support it.

zackdotcomputer avatar Sep 29 '21 03:09 zackdotcomputer