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-wrap-multiline eslint crash #2100

Closed
mjhost opened this issue Dec 30, 2018 · 4 comments
Closed

jsx-wrap-multiline eslint crash #2100

mjhost opened this issue Dec 30, 2018 · 4 comments
Assignees

Comments

@mjhost
Copy link

mjhost commented Dec 30, 2018

Thanks all for the great library, I use it a lot.
Recently I have discovered a problem with the rule jsx-wrap-multiline that throws exception during eslint execution.

I have a really bad formatted file:

import React from 'react';

export default () =>
    <div>
        with newline without parenthesys eslint crashes
    </div>

the error I receive is

TypeError: Cannot read property 'range' of null
    at Object.fixer [as fix] ([...]/node_modules/eslint-plugin-react/lib/rules/jsx-wrap-multilines.js:165:51)
...

I'll attach a basic example
jsx-wrap-multiline-issue.zip

There are a few ways of patching this error and I would like to send a PR for it, but I'm not sure how would you like the fix to be implemented. Also I'm not aware of possible implications.

Let me know if I can help you

@ljharb
Copy link
Member

ljharb commented Dec 30, 2018

https://github.com/yannickcr/eslint-plugin-react/blob/master/lib/rules/jsx-wrap-multilines.js#L165 implies that either the before or after token was null, and not being checked.

Added in #1576 / cc @sharmilajesupaul

@mjhost
Copy link
Author

mjhost commented Dec 30, 2018

in this case it was the after token

@mjhost
Copy link
Author

mjhost commented Dec 31, 2018

there's 3 fixes that I'm aware of:

  1. changing the condition here
    from tokenBefore.loc.end.line < node.loc.start.line
    to tokenBefore && tokenAfter && tokenBefore.loc.end.line < node.loc.start.line
  2. changing the parameter call of replaceTextRange here
    from [tokenBefore.range[0], tokenAfter.range[0]]
    to [tokenBefore.range[0], tokenAfter && tokenAfter.range[0]]
  3. changing the initialization of const tokenAfter here
    from const tokenAfter = sourceCode.getTokenAfter(node, {includeComments: true});
    to const tokenAfter = sourceCode.getTokenAfter(node, {includeComments: true}) || { range: [false /*or whatever*/] };

but maybe there's a smarter use of getTokenAfter (or maybe the bug is definitely there) or a smarter use of replaceTextRange that I'm not aware of that may be worth implementing.

anyway rule is yours, pick your favourite fix I'll be happy to send the PR.

any choice you pick --fix works like a charm, my choice would be digging on getTokenAfter or replaceTextRange but I really don't know where to start searching for those

@ljharb
Copy link
Member

ljharb commented Dec 31, 2018

I’m really not sure :-) as long as the fix comes with lots of test cases, at least some of which fail absent the fix, i think maybe 1 > 2 > 3?

@ljharb ljharb self-assigned this Jan 1, 2019
@ljharb ljharb closed this as completed in 695e534 Jan 1, 2019
This was referenced Jan 1, 2019
This was referenced Jan 7, 2019
@ghost ghost mentioned this issue Jan 12, 2019
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

2 participants