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

[anchor-is-valid] Allow Object spread in href/specialLink values

Open caub opened this issue 7 years ago • 14 comments

We get this error Cannot read property 'expression' of undefined as eslint output when using

const a = {};
const foo = () => (
	<Link
		to={{
			pathname: '/bar',
			query: { ...a },
		}}
	/>
);

and .eslintrc:

{
	"extends": [
		"eslint:recommended",
	],
	"plugins": ["jsx-a11y"],
	"env": {
		"browser": true,
		"node": true
	},
	"parserOptions": {
		"ecmaVersion": 2018,
		"sourceType": "module",
		"experimentalObjectRestSpread": true,
		"ecmaFeatures": {
			"jsx": true
		}
	},
	"rules": {
		"no-unused-vars": "warn",
		"jsx-a11y/anchor-is-valid": ["warn", {
			"components": ["Link"],
			"specialLink": [ "to" ],
			"aspects": ["noHref", "invalidHref", "preferButton"]
		}]
	}
}

here's a passing test:

{
  code: '<Anchor hrefLeft={{ foo: { test: 1 } } }} />',
  options: componentsAndSpecialLinkAndNoHrefAspect,
},

and a failing test, when object spread is used (even with experimentalObjectRestSpread in parserOptions or babel-eslint as parser with the right .babelrc..):

{
  code: '<Anchor hrefLeft={{ foo: { ...{ test: 1 } } }} />',
  options: componentsAndSpecialLinkAndNoHrefAspect,
},
Message:
      A fatal parsing error occurred: Parsing error: Unexpected token ...

I reported the error in eslint originally: https://github.com/eslint/eslint/issues/9960#issuecomment-365179752

@AlmeroSteyn I can try to solve it, if you have any advices maybe?

caub avatar Feb 13 '18 08:02 caub

I'm also experiencing this.

stefanmaric avatar May 02 '18 16:05 stefanmaric

This is an interesting request; a url is a string, and that’s what this is meant to enforce.

I think if react-router’s Link is sufficiently different from an <a>, then it warrants its own react-router-specific rule.

ljharb avatar May 02 '18 18:05 ljharb

@caub Wow sorry somehow missed the @ CC

Yes, as @ljharb mentions, it was not built to cater for objects in the link prop.

The React Router Link component renders to an accessible <a> tags and the to prop is guarded with a prop-type isRequired declaration and, when omitted, it will be reported on by React itself as can be seen on the following console capture:

image

I am not sure if this should be double checked by this linter as when the React error is dealt with, an accessible <a> will be rendered.

Thoughts?

AlmeroSteyn avatar May 03 '18 08:05 AlmeroSteyn

I created a repo repo with this issue before I realized there was already any issue. Here's the link if it helps anyone:

https://github.com/justinanastos/eslint-plugin-jsx-a11y-anchor-is-valid-error


That being said, I'd like to throw my opinion into the ring: whether or not this rule should accurately lint the to={[ Object ]} case, this lint plugin should not crash eslint. I think that would mean that using to={[Object ]} would make this rule incompatible with aspects: ['invalidHref'], which completely makes sense. One can chose to not include the invalidHref aspects in their configuration as long as this rule doesn't crash. Right now, if you include Link in components and then ever use an object in to=, this will crash eslint regardless of the other configuration options.

If the maintainers are interested in a workaround, I'd be happy to submit a PR; I just need some guidance on what would be acceptable.

justinanastos avatar May 22 '18 14:05 justinanastos

I absolutely agree that it should not crash the plugin; a PR that prevents that (without silently allowing code that failed to pass before) would be great.

ljharb avatar May 22 '18 19:05 ljharb

Any updates on this?

kaitlynbrown avatar Jan 01 '19 02:01 kaitlynbrown

No, nobody’s submitted a PR yet.

ljharb avatar Jan 01 '19 02:01 ljharb

Possibly related to https://github.com/evcohen/jsx-ast-utils/issues/69. Workaround involves extracting to into a variable and passing that variable to the component prop; e.g.:

const a = {};
const to = {
  pathname: '/bar',
  query: { ...a },
}
const foo = () => <Link to={to}/>;

Admittedly, not all that practical if you have a lot of to props with spread objects.

shanecav84 avatar Feb 12 '19 17:02 shanecav84

This one is really throwing me for a loop. I tried updating the parser config, but that doesn't solve the parser error. Updating eslint doesn't solve it either:

{
  extends: [
    "airbnb-base",
    "plugin:flowtype/recommended"
  ],
  parser: "babel-eslint",
  parserOptions: {
    ecmaVersion: 2018,
    ecmaFeatures: {
      experimentalObjectRestSpread: true,
    }
  },
}

Has anyone figured out the magical incantation to at least get the code to parse? Then I can fix the code error that occurs after that.

jessebeach avatar Mar 16 '19 23:03 jessebeach

Alright, this should get the test runner to at least parse the failing case. Now to fix the failing case :)

#573

jessebeach avatar Mar 16 '19 23:03 jessebeach

I think https://github.com/evcohen/jsx-ast-utils/pull/74 solves the Cannot read property 'expression' of undefined error that @caub notes in the OP.

EDIT: Nope, I just reproduced the OP. It's in evcohen/jsx-ast-utils. I'll get it fixed.

jessebeach avatar Mar 16 '19 23:03 jessebeach

Nice if it get fixed!

ps: I wonder if the spread error still happens with:

const a = {};
const foo = () => (
	<Link to={'/bar?' + new URLSearchParams({ ...a })} />
);

I guess it does

caub avatar Mar 17 '19 16:03 caub

I've got a pull against jsx-ast-utils to resolve the error: https://github.com/evcohen/jsx-ast-utils/pull/75

jessebeach avatar Mar 17 '19 19:03 jessebeach

@caub yes, that element pass as well with the fix:

./node_modules/.bin/jest ./__tests__/src/rules/anchor-is-valid-test.js
 PASS  __tests__/src/rules/anchor-is-valid-test.js
  anchor-is-valid
    valid
      ✓ <Link to={"/bar?" + new URLSearchParams({ ...a })} /> (59ms)

Test Suites: 1 passed, 1 total
Tests:       1 passed, 1 total
Snapshots:   0 total
Time:        1.126s, estimated 2s

jessebeach avatar Mar 17 '19 20:03 jessebeach