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

newline-after-import breaks again #441

Closed
ljharb opened this issue Jul 15, 2016 · 11 comments
Closed

newline-after-import breaks again #441

ljharb opened this issue Jul 15, 2016 · 11 comments
Labels
Milestone

Comments

@ljharb
Copy link
Member

ljharb commented Jul 15, 2016

Now that #386 is resolved, I'm now getting the following:

Cannot read property 'range' of undefined
TypeError: Cannot read property 'range' of undefined
    at containsNodeOrEqual ($PWD/node_modules/eslint-plugin-import/lib/rules/newline-after-import.js:23:19)
    at $PWD/node_modules/eslint-plugin-import/lib/rules/newline-after-import.js:111:34
    at Array.forEach (native)
    at $PWD/node_modules/eslint-plugin-import/lib/rules/newline-after-import.js:104:22
    at Array.forEach (native)
    at EventEmitter.ProgramExit ($PWD/node_modules/eslint-plugin-import/lib/rules/newline-after-import.js:100:14)
    at emitOne (events.js:82:20)
    at EventEmitter.emit (events.js:169:7)
    at NodeEventGenerator.leaveNode ($PWD/node_modules/eslint/lib/util/node-event-generator.js:49:22)
    at CodePathAnalyzer.leaveNode ($PWD/node_modules/eslint/lib/code-path-analysis/code-path-analyzer.js:627:23)
    at CommentEventGenerator.leaveNode ($PWD/node_modules/eslint/lib/util/comment-event-generator.js:110:23)
@benmosher
Copy link
Member

benmosher commented Jul 17, 2016

Seems to be () => require('x') && require('y') that gets into this state.

I'm disqualifying ArrowFunctionExpressions (more generally, single-statement scopes) from newline-after-import. Strictly an improvement, since they are currently broken.

benmosher added a commit that referenced this issue Jul 17, 2016
@ljharb
Copy link
Member Author

ljharb commented Jul 17, 2016

I don't have any && require or || require in my codebase, fwiw, so I'm not sure if this will fix the problem or not.

@benmosher
Copy link
Member

I'm not either. If it doesn't, it'd be ideal to have a failing test case.

This was the first thing I could come up with just syntax-ing around that threw the same exception, and while I could imagine other execution states that could throw the same error, I was unable to imagine syntax (and consequently, test cases) to go with them.

@benmosher
Copy link
Member

Published v1.11.0 to npm. Fingers crossed.

@benmosher benmosher added this to the patch milestone Jul 18, 2016
@ljharb
Copy link
Member Author

ljharb commented Jul 18, 2016

Sadly this still occurs. How can I help figure out which file is causing the issue?

@benmosher
Copy link
Member

Bummer. 😞

Best idea I have had: some sort of find . -exec eslint that stops when ESLint returns a nonzero exit code?

@ljharb
Copy link
Member Author

ljharb commented Jul 19, 2016

What about modifying the rule in eslint-plugin-import to console.log the filename being worked on, and i can just look for the last one? Do you know how I can get the filename from a scope?

@ljharb
Copy link
Member Author

ljharb commented Jul 19, 2016

@benmosher OK, I think I found it:

    switch (foo) {
      case '1':
        return require('../path/to/file1.jst.hbs')(renderData, options);
      case '2':
        return require('../path/to/file2.jst.hbs')(renderData, options);
      case '3':
        return require('../path/to/file3.jst.hbs')(renderData, options);
      case '4':
        return require('../path/to/file4.jst.hbs')(renderData, options);
      case '5':
        return require('../path/to/file5.jst.hbs')(renderData, options);
      case '6':
        return require('../path/to/file6.jst.hbs')(renderData, options);
      case '7':
        return require('../path/to/file7.jst.hbs')(renderData, options);
      case '8':
        return require('../path/to/file8.jst.hbs')(renderData, options);
      case '9':
        return require('../path/to/file9.jst.hbs')(renderData, options);
      case '10':
        return require('../path/to/file10.jst.hbs')(renderData, options);
      case '11':
        return require('../path/to/file11.jst.hbs')(renderData, options);
      case '12':
        return something();
      default:
        return somethingElse();
    }

@ljharb
Copy link
Member Author

ljharb commented Jul 19, 2016

It appears that statementWithRequireCall is undefined here: https://github.com/benmosher/eslint-plugin-import/blob/master/src/rules/newline-after-import.js#L100 - if i do an early return when that's falsy, it seems to fix the problem (but no idea if that's the right fix).

@benmosher
Copy link
Member

Makes sense. Same thing was happening with the arrows; the scope position comes back as -1 and thus the statement is undefined.

Thanks for the repro, I'll debug and try to trace it back from there. May add a debug logger to get the file name as you've suggested, as well.

benmosher added a commit that referenced this issue Jul 20, 2016
@benmosher
Copy link
Member

Turns out the sourceType matters for this. Added a test with the code you provided, and it passed. After playing with parserOptions and parser for a little bit, I discovered that the module parsing goal matters for this, for reasons I don't understand.

Will publish as 1.11.1, and then we can reopen if it's still broken (for this or new reasons).

Did add logging as well, so DEBUG=eslint-plugin-import:* eslint [...] should print out some helpful messages from now on, for this rule.

@benmosher benmosher mentioned this issue Jul 20, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

No branches or pull requests

2 participants