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

Crashes on ESLint require-jsdoc directive comments #1090

Open
jaydenseric opened this issue Jun 12, 2018 · 4 comments
Open

Crashes on ESLint require-jsdoc directive comments #1090

jaydenseric opened this issue Jun 12, 2018 · 4 comments

Comments

@jaydenseric
Copy link
Contributor

This bug confused the heck out of me!

This:

// eslint-disable-next-line require-jsdoc
const Foo = () => <div />

Or this:

/* eslint-disable require-jsdoc */
const Foo = () => <div />
/* eslint-enable require-jsdoc */

Will cause the documentation.js CLI to crash for either lint or readme operations with an error similar to this example:

Error: Parsing file /Users/jaydenseric/Sites/graphql-react/src/test.mjs: Unexpected token (130:4)
    at Deps.parseDeps (/Users/jaydenseric/Sites/graphql-react/node_modules/module-deps-sortable/index.js:467:28)
    at fromSource (/Users/jaydenseric/Sites/graphql-react/node_modules/module-deps-sortable/index.js:402:44)
    at /Users/jaydenseric/Sites/graphql-react/node_modules/module-deps-sortable/index.js:396:17
    at ConcatStream.<anonymous> (/Users/jaydenseric/Sites/graphql-react/node_modules/module-deps-sortable/node_modules/concat-stream/index.js:36:43)
    at ConcatStream.emit (events.js:187:15)
    at finishMaybe (/Users/jaydenseric/Sites/graphql-react/node_modules/module-deps-sortable/node_modules/concat-stream/node_modules/readable-stream/lib/_stream_writable.js:475:14)
    at endWritable (/Users/jaydenseric/Sites/graphql-react/node_modules/module-deps-sortable/node_modules/concat-stream/node_modules/readable-stream/lib/_stream_writable.js:485:3)
    at ConcatStream.Writable.end (/Users/jaydenseric/Sites/graphql-react/node_modules/module-deps-sortable/node_modules/concat-stream/node_modules/readable-stream/lib/_stream_writable.js:455:41)
    at DuplexWrapper.onend (/Users/jaydenseric/Sites/graphql-react/node_modules/readable-stream/lib/_stream_readable.js:577:10)
    at Object.onceWrapper (events.js:273:13)

Take away the comments and it works fine. I think the require part of the comment is erroneously causing some sort of import analysis to happen, which happens to fail because there is JSX in the file. Maybe this accidental analysis would go unnoticed if it weren't for the JSX, or maybe it would error on something else. I don't know.

  • What version of documentation.js are you using?: v8.0.0
  • How are you running documentation.js (on the CLI, Node.js API, Grunt, other?): CLI
@jaydenseric
Copy link
Contributor Author

Here is the root cause: browserify/detective#45

detective is used by module-deps here: https://github.com/browserify/module-deps/blob/v6.1.0/index.js#L506

And I think module-deps is used indirectly here: https://github.com/documentationjs/documentation/blob/v8.0.0/src/input/dependency.js#L25

@jaydenseric
Copy link
Contributor Author

This seems to only happen when the source file has a .mjs extension.

@jaydenseric
Copy link
Contributor Author

jaydenseric commented Jun 13, 2018

The babelify transform is not transforming source from .mjs files for some reason. I've created a dirty noop transformer to log out what is happening:

const through = require('through2')

function (file) {
  return through(function (buf, enc, next) {
      const str = buf.toString('utf8')
      console.log(str)
      this.push(str)
      next()
  });
}

Adding it after the babelify transform here reveals that when the extention is .js it babelify has transformed the source, but not when it is .mjs.

I've checked the result of the filter and postFilter mdeps options and the .mjs files are not being filtered out, so something else is filtering them.

@jaydenseric
Copy link
Contributor Author

jaydenseric commented Jun 13, 2018

Idea: Maybe we need to use the mdeps resolve option, because the default node-browser-resolve in turn uses a pinned, old version of resolve, which itself is out of touch with the actual node resolution algorithm and doesn't appear to support .mjs.

I don't really understand how this all fits together though, since I though we were force feeding the transformer all the files gathered by a glob, instead of an index with imports that needs to be recursively resolved.

I get the feeling it would be better to use Babel APIs directly instead of this dated browserify setup. I'm not sure why transpilation is actually necessary at all, since there are parsers available that can understand modern syntax and even JSX.

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

No branches or pull requests

1 participant