Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

jsx-curly-brace-presence fails with typeError #2423

Closed
marusak opened this issue Oct 1, 2019 · 6 comments
Closed

jsx-curly-brace-presence fails with typeError #2423

marusak opened this issue Oct 1, 2019 · 6 comments

Comments

@marusak
Copy link

marusak commented Oct 1, 2019

In the newest version (7.15.0) we get unexpected TypeError. Does not happen with previous version. Our .eslintrc.json config.
Either disabling react/jsx-curly-brace-presence rule on pinning down 7.14.3 version is quick workaround if anyone else sees this.

$ cat foo.jsx 
import React from "react";
import { OverlayTrigger, Tooltip } from "patternfly-react";

export class Foo extends React.Component {
    render() {
        return (
            <OverlayTrigger overlay={ <Tooltip id="tip-service">Foo</Tooltip> } />
        );
    }
}
$ eslint foo.jsx 
TypeError: Cannot read property 'substring' of undefined
Occurred while linting /home/mmarusak/cockpit/cockpit1/foo.jsx:7
    at Object.fix (/home/mmarusak/cockpit/cockpit1/node_modules/eslint-plugin-react/lib/rules/jsx-curly-brace-presence.js:129:30)
    at normalizeFixes (/usr/lib/node_modules/eslint/lib/linter/report-translator.js:176:28)
    at args (/usr/lib/node_modules/eslint/lib/linter/report-translator.js:278:49)
    at Object.report (/usr/lib/node_modules/eslint/lib/linter/linter.js:904:41)
    at reportUnnecessaryCurly (/home/mmarusak/cockpit/cockpit1/node_modules/eslint-plugin-react/lib/rules/jsx-curly-brace-presence.js:117:15)
    at lintUnnecessaryCurly (/home/mmarusak/cockpit/cockpit1/node_modules/eslint-plugin-react/lib/rules/jsx-curly-brace-presence.js:198:9)
    at JSXExpressionContainer (/home/mmarusak/cockpit/cockpit1/node_modules/eslint-plugin-react/lib/rules/jsx-curly-brace-presence.js:278:11)
    at listeners.(anonymous function).forEach.listener (/usr/lib/node_modules/eslint/lib/linter/safe-emitter.js:45:58)
    at Array.forEach (<anonymous>)
    at Object.emit (/usr/lib/node_modules/eslint/lib/linter/safe-emitter.js:45:38)
@ljharb
Copy link
Member

ljharb commented Oct 1, 2019

This was introduced in #2409 by @vedadeepta. Thanks for the great test case.

@vedadeepta, can you take a look at this today?

@ljharb ljharb closed this as completed in feba507 Oct 1, 2019
@ljharb
Copy link
Member

ljharb commented Oct 1, 2019

I went ahead and pushed a quick fix to this; we'll want to follow up with a proper fix that actually reports on jsx elements inside props.

@vedadeepta
Copy link
Contributor

vedadeepta commented Oct 1, 2019

@marusak not able to reproduce this issue - can you provide a sandbox link ?
sorry i forgot to pull the latest

@ljharb
Copy link
Member

ljharb commented Oct 1, 2019

@vedadeepta see the test case i added in feba507, but without my workaround/fix.

@vedadeepta
Copy link
Contributor

we'll want to follow up with a proper fix that actually reports on jsx elements inside props.

I think curly braces for jsx elements inside prop should always be required.

<App elm={<CustomElement>.....</CustomElement>} elm1={<Custom />} />

so maybe the test should detect and skip curly braces check if the element inside it is a jsx-element.

Let me know if you think otherwise or if any special cases.

@ljharb
Copy link
Member

ljharb commented Oct 1, 2019

@vedadeepta yes, they are syntactically always required for any non-string value - so i think the better fix is to bail out entirely when the contents of the curly braces aren't a string literal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

3 participants