Skip to content

eslint-module-utils: filePath in parserOptions #840

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

Merged
merged 8 commits into from
May 31, 2017
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@ This project adheres to [Semantic Versioning](http://semver.org/).
This change log adheres to standards from [Keep a CHANGELOG](http://keepachangelog.com).

## [Unreleased]

### Added
- Add `filePath` into `parserOptions` passed to `parser` ([#839], thanks [@sompylasar])

## [2.3.0] - 2017-05-18
### Added
Expand Down
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@
"nyc": "^8.3.0",
"redux": "^3.0.4",
"rimraf": "2.5.2",
"sinon": "^2.3.2",
"typescript": "^2.0.3",
"typescript-eslint-parser": "^2.1.0"
},
Expand Down
34 changes: 34 additions & 0 deletions tests/src/core/parse.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import * as fs from 'fs'
import { expect } from 'chai'
import sinon from 'sinon'
import parse from 'eslint-module-utils/parse'

import { getFilename } from '../utils'
Expand All @@ -21,4 +22,37 @@ describe('parse(content, { settings, ecmaFeatures })', function () {
.not.to.throw(Error)
})

it('passes expected parserOptions to custom parser', function () {
const parseSpy = sinon.spy()
const parserOptions = { ecmaFeatures: { jsx: true } }
require('./parseStubParser').parse = parseSpy
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

require has a cache, so you get the same value out of it every time - any reason this isn't required at the top level?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, can pull to top. Both this and resolve. Just a habit of having self-contained tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done: 5732742

parse(path, content, { settings: {}, parserPath: require.resolve('./parseStubParser'), parserOptions: parserOptions })
expect(parseSpy.callCount, 'custom parser to be called once').to.equal(1)
expect(parseSpy.args[0][0], 'custom parser to get content as its first argument').to.equal(content)
expect(parseSpy.args[0][1], 'custom parser to get an object as its second argument').to.be.an('object')
expect(parseSpy.args[0][1], 'custom parser to clone the parserOptions object').to.not.equal(parserOptions)
expect(parseSpy.args[0][1], 'custom parser to get ecmaFeatures in parserOptions which is a clone of ecmaFeatures passed in')
.to.have.property('ecmaFeatures')
.that.is.eql(parserOptions.ecmaFeatures)
.and.is.not.equal(parserOptions.ecmaFeatures)
expect(parseSpy.args[0][1], 'custom parser to get parserOptions.attachComment equal to true').to.have.property('attachComment', true)
expect(parseSpy.args[0][1], 'custom parser to get parserOptions.filePath equal to the full path of the source file').to.have.property('filePath', path)
})

it('should throw on context == null', function () {
expect(parse.bind(null, path, content, null)).to.throw(Error)
})

it('should throw on unable to resolve parserPath', function () {
expect(parse.bind(null, path, content, { settings: {}, parserPath: null })).to.throw(Error)
})

it('should take the alternate parser specified in settings', function () {
const parseSpy = sinon.spy()
const parserOptions = { ecmaFeatures: { jsx: true } }
require('./parseStubParser').parse = parseSpy
expect(parse.bind(null, path, content, { settings: { 'import/parsers': { [require.resolve('./parseStubParser')]: [ '.js' ] } }, parserPath: null, parserOptions: parserOptions })).not.to.throw(Error)
expect(parseSpy.callCount, 'custom parser to be called once').to.equal(1)
})

})
4 changes: 4 additions & 0 deletions tests/src/core/parseStubParser.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
// this stub must be in a separate file to require from parse via moduleRequire
module.exports = {
parse: function () {},
}
8 changes: 6 additions & 2 deletions utils/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,13 @@ All notable changes to this module will be documented in this file.
This project adheres to [Semantic Versioning](http://semver.org/).
This change log adheres to standards from [Keep a CHANGELOG](http://keepachangelog.com).

## v2 - 2016-11-07
## [Unreleased]
### Added
- `parse` now additionally passes `filePath` to `parser` in `parserOptions` like `eslint` core does

## v2.0.0 - 2016-11-07
### Changed
- `unambiguous` no longer exposes fast test regex

### Fixed
- `unambiguous.test()` regex is now properly in multiline mode
- `unambiguous.test()` regex is now properly in multiline mode
4 changes: 4 additions & 0 deletions utils/parse.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,10 @@ exports.default = function parse(path, content, context) {
// always attach comments
parserOptions.attachComment = true

// provide the `filePath` like eslint itself does, in `parserOptions`
// https://github.com/eslint/eslint/blob/3ec436ee/lib/linter.js#L637
parserOptions.filePath = path
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps the path needs to be path.normalized?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe. What is the error case? The serialized parserOptions participates in some cache key, right? Where is it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The internal parser has a cache that is a hash of the parse settings and the module path. So I'm guessing the path must be relative.

An absolute path would work, or the cache key could drop the file path key.

I am not at a computer right now but I think the cache access is in utils/resolve.js

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, not thinking -- relative would be fine as long as it's all against cwd. So maybe path.normalize would be fine

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the pointers, I'll look into that and suggest a change and tests when I get to my computer (afk, too).


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

Expand Down