import-js / eslint-plugin-import

ESLint plugin with rules that help validate proper imports.
MIT License
5.55k stars 1.57k forks source link

TypeError: Cannot read property 'some' of undefined when `ast.comments` is missing on the AST #842

Closed sompylasar closed 7 years ago

sompylasar commented 7 years ago

https://github.com/benmosher/eslint-plugin-import/blob/1377f55bc3d003519a74a89b048e11b20d53990e/src/ExportMap.js#L333

ExportMap.parse = function (path, content, context) {
  // ...

  // attempt to collect module doc
  ast.comments.some(c => {
ljharb commented 7 years ago

What code generates this error?

sompylasar commented 7 years ago

@ljharb I'm still investigating, but for some reason typescript-eslint-parser which I'm experimenting with returned me an AST without .comments on it.

ljharb commented 7 years ago

If that's a guarantee that other tools provide, then it might be a bug with typescript-eslint-parser.

sompylasar commented 7 years ago

@ljharb OK, let's keep it as is then, no change required.

I have accidentally fed up JavaScript code to typescript-eslint-parser instead of babel-eslint, and for some reason the comments weren't extracted.

All that because while waiting for https://github.com/benmosher/eslint-plugin-import/issues/839 to be merged and published I'm trying to detect TypeScript code by file contents, and my naive detection failed.

ljharb commented 7 years ago

I'd recommend using the extension, since the sole purpose of file extensions is to tell you how to parse a file :-)

sompylasar commented 7 years ago

@ljharb Definitely! But I have no filePath where I need it yet to get the file extension from (this https://github.com/benmosher/eslint-plugin-import/issues/839 solves it, but it's not released yet so I cannot install it as a dependency).

samhh commented 7 years ago

I just hit this same issue with typescript-eslint-parser. Am I to follow #839 for updates on a fix?

sompylasar commented 7 years ago

@SamHH Depends on what you're trying to do. The https://github.com/benmosher/eslint-plugin-import/issues/839 makes it possible for me to provide ESLint with a custom parser which can parse both TypeScript and JavaScript while Babel/Babylon is lagging behind:

// ./webpack/parserForEslint.js

function shouldUseTypeScriptParser(text, parserOptions) {
  return isTypeScriptFilePath(parserOptions.filePath);
}

// ...

module.exports = {
  parse: function (text, parserOptions) {
    if (shouldUseTypeScriptParser(text, parserOptions)) {
      return parseTypeScript(text, parserOptions);
    }

    return parseJavaScript(text, parserOptions);
  },
};
// .eslintrc.js
  // ...
  "parser": "./webpack/parserForEslint.js",
  // ...
mattdell commented 7 years ago

I had this issue when accidentally trying to import a JS file in a TS file.

sompylasar commented 7 years ago

@mattdell Yes, looks like typescript-eslint-parser cannot parse JS (only TS), and that's fine.

You'll have to do the same as I did for a JS+TS project: a conditional parser that decides which real parser to use.

This conditional parser requires filePath in parserOptions to reliably guess the type if the code it's been given for parsing.

But eslint-plugin-import doesn't provide this yet, hence #839.

piecyk commented 7 years ago

hmm got the same error, using eslint-plugin-import@2.3.0 and typescript-eslint-parser@3.0.0 working on JS+TS project, why we need conditional parser @sompylasar ?

i was thinking that import/parsers will do that job ?

'import/parsers': {
  'typescript-eslint-parser': ['.ts', '.tsx']
},
sompylasar commented 7 years ago

@piecyk Because either I missed this import/parsers option, or it wasn't there at the time I was setting this up. Thanks for the pointer.

piecyk commented 7 years ago

@sompylasar 👍

Was looking on this issue because after upgrading to typescript-eslint-parser@3.0.0 the ast.comments for ts, tsx files is undefined and got the same error...

piecyk commented 7 years ago

oki problem was that i didn't set comment in parserOptions in .eslintrc as typescript-eslint-parser requires https://github.com/eslint/typescript-eslint-parser/blob/master/parser.js#L30

that option is passed to eslint-plugin-import and we got ast.comments

sompylasar commented 7 years ago

@piecyk The thing is that the parserOptions in eslint-plugin-import (eslint-module-utils) get extended with the attachComment option, and not with comment option. https://github.com/benmosher/eslint-plugin-import/blob/d92ef4365b55495dbcce8677f5cbfd08d023440a/utils/parse.js#L22-L23

You're saying typescript-eslint-parser does not use the attachComment option, rather it requires comment option instead. So there is an incompatibility between different eslint parsers which has to be fixed.

babel-eslint provides both: https://github.com/babel/babel-eslint/blob/37f9242fc6979fdd90e22dd26ef5b1ca7905b486/test/babel-eslint.js#L78-L79 to espree which documents them as follows:

    // create a top-level comments array containing all comments
    comment: false,

    // attach comments to the closest relevant node as leadingComments and trailingComments
    attachComment: false,
piecyk commented 7 years ago

@sompylasar hmm based on espree docs in this situation {comment: true, attachComment: false} is ast.comments populated ?

so now when eslint-plugin-import run with espree ( as default ) will run with {comment: false, attachComment: true} and will create comments

strange 🤔 hard to say what is best approach here because don't grasp exact difference between comment vs attachComment

sompylasar commented 7 years ago

@piecyk The difference is (if you know what AST looks like), in pseudo code:

// comment
const ast = espree.parse(...);
ast.comments  // -> array
// attachComment
const ast = espree.parse(...);

ast.body[0].leadingComments  // -> array
ast.body[0].trailingComments  // -> array

types.visit(ast, {
  visitFunctionExpression: (path) => {
    const node = path.value;
    node.leadingComments  // -> array
    node.trailingComments  // -> array
  },
})

I have no time or a repro right now to track down the issue, sorry. If you have the repro of the error and the curiosity, please find that out.

piecyk commented 7 years ago

@sompylasar thanks 👍 will check that out ( and read more about AST internals )

from quick look, basic typescript-eslint-parser drooped attachComment https://github.com/eslint/typescript-eslint-parser/pull/219

so this https://github.com/benmosher/eslint-plugin-import/blob/master/src/ExportMap.js#L182 should take under consideration that attachComment is not valid, but comments can be there ?

sompylasar commented 7 years ago

@piecyk The captureDoc function you mentioned handles missing leadingComments and does not use trailingComments. So it kinda expects the product of attachComment, but does not strictly require it to be present.

But the ExportMap.parse function linked in the description expects ast.comments to be always present: ast.comments.some(c => {.

So in fact eslint-plugin-import depends on the product of and should extend the parserOptions with the comment option here (in addition to attachComment for the captureDoc to keep working).

CC @ljharb


P.S. Please link to commits, not branches (i.e. /master/); to do that, press y on your keyboard before copying the link to the source code.

sompylasar commented 7 years ago

@piecyk I realized why I need a conditional parser, and why import/parsers is not enough: I need to provide eslint itself with a parser which understands both JavaScript and TypeScript, not just the eslint-plugin-import. This parser must return the same AST for the same code to both the eslint core parse and the eslint-plugin-import parse. While waiting for Babel/Babylon to understand TypeScript, this seems to be the only way.

OliverJAsh commented 7 years ago

Got this error using TypeScript 2.4.1, eslint-plugin-import 2.7.0, eslint-plugin-typescript 0.1.0, and typescript-eslint-parser 3.0.0.

I'm having a hard time understanding what the fix is. I have this in my ESLint config:

  settings: {
    'import/parsers': {
      'typescript-eslint-parser': ['.ts', '.tsx'],
    },
}

Can anyone provide a conclusion of what the fix is?

sompylasar commented 7 years ago

The issue with the eslint-plugin-import code is still valid, see https://github.com/benmosher/eslint-plugin-import/issues/842#issuecomment-310243434 for details – reopening.

OliverJAsh commented 7 years ago

Is there any workaround?

I seemed to have some success with setting comment: true parser option.

sompylasar commented 7 years ago

@OliverJAsh Yes, setting comment: true in the parser options provided from the outside should do the trick while eslint-plugin-import does not do that from the inside.

alexgorbatchev commented 7 years ago

The fix appears to be pretty trivial, unless there's something else going on. Can I just submit a PR with if (ast.comments) ... added? This issue is killing me and adding { comment: true } doesn't work for me.

You can reproduce the issue with: https://github.com/alexgorbatchev/typescript-module-boilerplate/tree/0cc9383c82de065b2a9f5f2d487004a28d97b30f and by running npm run lint

alexgorbatchev commented 7 years ago

ping ☝️

ljharb commented 7 years ago

Might as well submit it :-)

alexgorbatchev commented 7 years ago

Done ☝️

alexgorbatchev commented 7 years ago

Would really like to see this published. This error makes the whole experience of linting in editor like VSCode completely broken because plugin exceptions end up as alerts. So any time I save and linter runs, i get exceptions. Had to disable linting for .tsx files all together for now.