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

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

Closed
sompylasar opened this issue May 22, 2017 · 29 comments

Comments

@sompylasar
Copy link
Contributor

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
Copy link
Member

ljharb commented May 22, 2017

What code generates this error?

@sompylasar
Copy link
Contributor Author

@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
Copy link
Member

ljharb commented May 22, 2017

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

@sompylasar
Copy link
Contributor Author

@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 #839 to be merged and published I'm trying to detect TypeScript code by file contents, and my naive detection failed.

@ljharb
Copy link
Member

ljharb commented May 22, 2017

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

@sompylasar
Copy link
Contributor Author

@ljharb Definitely! But I have no filePath where I need it yet to get the file extension from (this #839 solves it, but it's not released yet so I cannot install it as a dependency).

@samhh
Copy link

samhh commented May 25, 2017

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

@sompylasar
Copy link
Contributor Author

@samhh Depends on what you're trying to do. The #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
Copy link

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

@sompylasar
Copy link
Contributor Author

@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
Copy link

piecyk commented Jun 21, 2017

hmm got the same error, using [email protected] and [email protected]
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
Copy link
Contributor Author

@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
Copy link

piecyk commented Jun 21, 2017

@sompylasar 👍

Was looking on this issue because after upgrading to [email protected] the ast.comments for ts, tsx files is undefined and got the same error...

@piecyk
Copy link

piecyk commented Jun 21, 2017

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
Copy link
Contributor Author

@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
Copy link

piecyk commented Jun 21, 2017

@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
Copy link
Contributor Author

sompylasar commented Jun 21, 2017

@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
Copy link

piecyk commented Jun 21, 2017

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

from quick look, basic typescript-eslint-parser drooped attachComment
eslint/typescript-eslint-parser#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
Copy link
Contributor Author

sompylasar commented Jun 22, 2017

@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
Copy link
Contributor Author

@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
Copy link

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
Copy link
Contributor Author

The issue with the eslint-plugin-import code is still valid, see #842 (comment) for details – reopening.

@sompylasar sompylasar reopened this Jul 6, 2017
@OliverJAsh
Copy link

Is there any workaround?

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

@sompylasar
Copy link
Contributor Author

@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
Copy link
Contributor

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
Copy link
Contributor

ping ☝️

@ljharb
Copy link
Member

ljharb commented Oct 12, 2017

Might as well submit it :-)

alexgorbatchev added a commit to alexgorbatchev/eslint-plugin-import that referenced this issue Oct 13, 2017
@alexgorbatchev
Copy link
Contributor

Done ☝️

@alexgorbatchev
Copy link
Contributor

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.

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

No branches or pull requests

7 participants