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

filePath is not provided to parser in parserOptions #839

Closed
sompylasar opened this issue May 19, 2017 · 6 comments
Closed

filePath is not provided to parser in parserOptions #839

sompylasar opened this issue May 19, 2017 · 6 comments

Comments

@sompylasar
Copy link
Contributor

I'm trying to make a conditional eslint parser which decides based on filename which parser to execute: Babel or TypeScript (because my project will soon contain both types of files which need to live together under a single eslint config for some time).

The native eslint parse call provides filePath in the parserOptions, which turned out the only and the very useful way to determine the file name:
https://github.com/eslint/eslint/blob/3ec436eeed0b0271e2ed0d0cb22e4246eb15f137/lib/linter.js#L637

function parse(text, config, filePath, messages) {

    let parser,
        parserOptions = {
            loc: true,
            range: true,
            raw: true,
            tokens: true,
            comment: true,
            filePath
        };

But when eslint-plugin-import parses its dependencies, it calls the custom parser on its own, and does not provide filePath in parserOptions:
https://github.com/benmosher/eslint-plugin-import/blob/90ef48b3ade57c77526b285f75dc0cfc41537831/utils/parse.js#L28

exports.default = function parse(path, content, context) {

  if (context == null) throw new Error('need context to parse properly')

  let parserOptions = context.parserOptions
  const parserPath = getParserPath(path, context)

  if (!parserPath) throw new Error('parserPath is required!')

  // hack: espree blows up with frozen options
  parserOptions = Object.assign({}, parserOptions)
  parserOptions.ecmaFeatures = Object.assign({}, parserOptions.ecmaFeatures)

  // always attach comments
  parserOptions.attachComment = true

  // require the parser relative to the main module (i.e., ESLint)
  const parser = moduleRequire(parserPath)

  return parser.parse(content, parserOptions)
}

This should not be a big change to add that, I'll propose a PR soon.

@benmosher
Copy link
Member

makes sense to me 👍

benmosher pushed a commit that referenced this issue May 31, 2017
* eslint-module-utils: filePath in parserOptions

Refs #839

* eslint-module-utils: Add tests for parserOptions

Refs #839

* eslint-module-utils: Reverted manual version bumps.

Refs #839

* Add sinon, replace eslint-module-utils test spy with sinon.spy

* Fix CHANGELOG merge error

* eslint-module-utils: Add more tests for parse (coverage 100%)

* eslint-module-utils: In tests move require stub parser to the top.
@benmosher
Copy link
Member

published under v2.4.0, thanks!

@benmosher
Copy link
Member

uh oh, seems to break the cache! not sure why this isn't caught by tests but it introduces a substantial perf + memory regression. unpublishing v2.1.0 of eslint-module-utils!

@sompylasar
Copy link
Contributor Author

@benmosher What? 0_o Which cache? How did you spot the regression?

@sompylasar
Copy link
Contributor Author

Cache issue investigation here: #863

@benmosher
Copy link
Member

benmosher commented Jun 22, 2017

reiterating here: I determined the leak is in memo-parser which I am probably the only person using. (or we'll find out soon enough)

republished with v2.1.1 of utils 👍🏻

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

2 participants