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

eslint-module-utils: filePath in parserOptions #840

Merged
merged 8 commits into from
May 31, 2017
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
36 changes: 36 additions & 0 deletions tests/src/core/parse.js
Original file line number Diff line number Diff line change
@@ -1,11 +1,14 @@
import * as fs from 'fs'
import { expect } from 'chai'
import sinon from 'sinon'
import parse from 'eslint-module-utils/parse'

import { getFilename } from '../utils'

describe('parse(content, { settings, ecmaFeatures })', function () {
const path = getFilename('jsx.js')
const parseStubParser = require('./parseStubParser')
const parseStubParserPath = require.resolve('./parseStubParser')
Copy link
Member

Choose a reason for hiding this comment

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

requires should generally all only be static and at the top level of the file - can these be moved one level higher?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ljharb This require was left here like that on purpose, this is not just a dependency like any other, this is a stub which must be in a file that can be literally required because of the way eslint, and following it, eslint-import-resolver, resolves the parser function via require function (and we can do nothing about that).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ljharb For the same reason, the line above: const path = getFilename('jsx.js') could be moved up, but it's not.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I understand, but this can stay where it is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In other words, require('./parseStubParser') and require.resolve('./parseStubParser') should sit next to each other, they are closely related in this test.

let content

before((done) =>
Expand All @@ -21,4 +24,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 } }
parseStubParser.parse = parseSpy
parse(path, content, { settings: {}, parserPath: parseStubParserPath, 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 } }
parseStubParser.parse = parseSpy
expect(parse.bind(null, path, content, { settings: { 'import/parsers': { [parseStubParserPath]: [ '.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