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 #386

Closed
ljharb opened this issue Jun 20, 2016 · 9 comments · Fixed by #395 or Urigo/tortilla#68
Closed

newline-after-import breaks #386

ljharb opened this issue Jun 20, 2016 · 9 comments · Fixed by #395 or Urigo/tortilla#68
Labels

Comments

@ljharb
Copy link
Member

ljharb commented Jun 20, 2016

Cannot read property 'type' of undefined
TypeError: Cannot read property 'type' of undefined
    at getScopeBody ($PWD/node_modules/eslint-plugin-import/lib/rules/newline-after-import.js:30:11)
    at $PWD/node_modules/eslint-plugin-import/lib/rules/newline-after-import.js:104:27
    at Array.forEach (native)
    at $PWD/node_modules/eslint-plugin-import/lib/rules/newline-after-import.js:103:22
    at Array.forEach (native)
    at EventEmitter.ProgramExit ($PWD/node_modules/eslint-plugin-import/lib/rules/newline-after-import.js:99: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)

No idea what file it breaks on, but it started immediately after enabling this rule.

@benmosher benmosher added the bug label Jun 21, 2016
@benmosher benmosher added this to the v1.9.2 milestone Jun 21, 2016
@benmosher
Copy link
Member

benmosher commented Jun 21, 2016

Sounds like somehow you've got a scope that isn't a BlockStatement.

I tried to conjure such a line. Couldn't figure it out.

Without a reproduction, I'm not sure how to proceed.

@benmosher benmosher removed this from the v1.9.2 milestone Jun 21, 2016
@jfmengels
Copy link
Collaborator

Have trouble reproducing it too 😕

@ljharb
Copy link
Member Author

ljharb commented Jun 21, 2016

It would be awesome if there was a way for the current file path to be part of the stack trace :-/

Could the code here perhaps just check if it's a BlockStatement, and explicitly throw, with the filename, if it's not, rather than relying on the assumption that that's what will be there?

@jfmengels
Copy link
Collaborator

That sounds like something that would be better integrated directly in ESLint.

To debug your use-case, you could do what you said in your local node_modules. That sounds a lot simpler to me than to release a new version for this. You can use context.getFilename() to get the name of the currently treated file (http://eslint.org/docs/developer-guide/working-with-rules#the-context-object).

If you could help us by doing that, that'd be great!

@sindresorhus
Copy link

Someone posted an issue about this on XO: xojs/xo#128

@sindresorhus
Copy link

It would be awesome if there was a way for the current file path to be part of the stack trace

@ljharb 👍 That should definitely be added to ESLint. Can you open an issue there? It's indeed pretty annoying debugging broken rules when you have no idea where the error is coming from.

@ljharb
Copy link
Member Author

ljharb commented Jun 23, 2016

@jfmengels the place where the error happens doesn't actually have context available - so in fact I do think releasing a new version that allows for better debugging is worth it, especially when the original rule had assumptions that weren't being unit tested.

That said, I'll try to play with the code in my local node_modules and see what I can come up with.

@ljharb
Copy link
Member Author

ljharb commented Jun 23, 2016

k, I've figured it out - there's a switch statement in our code that looks like this:

  switch (renderData.modalViewKey) {
      case 'a':
        return require('../path/to/template.hbs')(renderData, options);
      case 'b':
        return require('../path/to/template.hbs')(renderData, options);
      case 'c':
        return require('../path/to/template.hbs')(renderData, options);
      case 'd':
        return require('../path/to/template.hbs')(renderData, options);
      case 'e':
        return require('../path/to/template.hbs')(renderData, options);
      case 'f':
        return require('../path/to/template.hbs')(renderData, options);
      case 'g':
        return require('../path/to/template.hbs')(renderData, options);
      case 'h':
        return require('../path/to/template.hbs')(renderData, options);
      case 'i':
        return require('../path/to/template.hbs')(renderData, options);
      case 'j':
        return require('../path/to/template.hbs')(renderData, options);
      case 'k':
        return require('../path/to/template.hbs')(renderData, options);
      case SOME_CONSTANT:
        return renderData.mainModalContent.clone();
      default:
        return renderData.mainModalContent.clone();
    }

When I comment this out, it doesn't error out.

@jfmengels
Copy link
Collaborator

Made a fix in #395.

jaylaw81 pushed a commit to appirio-digital/ads-best-practices that referenced this issue Sep 19, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4 participants