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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ This change log adheres to standards from [Keep a CHANGELOG](http://keepachangel
- [`no-anonymous-default-export`] rule: report anonymous default exports ([#712], thanks [@duncanbeevers]).
- Add new value to [`order`]'s `newlines-between` option to allow newlines inside import groups ([#627], [#628], thanks [@giodamelio])
- Add `count` option to the [`newline-after-import`] rule to allow configuration of number of newlines expected ([#742], thanks [@ntdb])
- Add `filePath` into `parserOptions` passed to `parser` ([#839], thanks [@sompylasar])

### Changed
- [`no-extraneous-dependencies`]: use `read-pkg-up` to simplify finding + loading `package.json` ([#680], thanks [@wtgtybhertgeghgtwtg])
Expand Down
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@
"debug": "^2.2.0",
"doctrine": "1.5.0",
"eslint-import-resolver-node": "^0.2.0",
"eslint-module-utils": "^2.0.0",
"eslint-module-utils": "^2.1.0",
"has": "^1.0.1",
"lodash.cond": "^4.3.0",
"minimatch": "^3.0.3",
Expand Down
19 changes: 18 additions & 1 deletion tests/src/core/parse.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import * as fs from 'fs'
import { expect } from 'chai'
import parse from 'eslint-module-utils/parse'

import { getFilename } from '../utils'
import { getFilename, makeNaiveSpy } from '../utils'

describe('parse(content, { settings, ecmaFeatures })', function () {
const path = getFilename('jsx.js')
Expand All @@ -21,4 +21,21 @@ describe('parse(content, { settings, ecmaFeatures })', function () {
.not.to.throw(Error)
})

it('passes expected parserOptions to custom parser', function () {
const parseSpy = makeNaiveSpy()
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).to.equal(1)
expect(parseSpy.lastCallArguments[0]).to.equal(content)
expect(parseSpy.lastCallArguments[1]).to.be.an('object')
expect(parseSpy.lastCallArguments[1]).to.not.equal(parserOptions)
expect(parseSpy.lastCallArguments[1])
.to.have.property('ecmaFeatures')
.that.is.eql(parserOptions.ecmaFeatures)
.and.is.not.equal(parserOptions.ecmaFeatures)
expect(parseSpy.lastCallArguments[1]).to.have.property('attachComment', true)
expect(parseSpy.lastCallArguments[1]).to.have.property('filePath', path)
})

})
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 () {},
}
14 changes: 14 additions & 0 deletions tests/src/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,20 @@ export function getFilename(file) {
return path.join(__dirname, '..', 'files', file || 'foo.js')
}

/**
* naive implementation of a function spy
* for more robust spy, consider replacing with sinon or chai-spies
* @return {function}
*/
export function makeNaiveSpy() {
const spy = function () {
spy.callCount += 1
spy.lastCallArguments = arguments
}
spy.callCount = 0
return spy
}

/**
* to be added as valid cases just to ensure no nullable fields are going
* to crash at runtinme
Expand Down
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
## v2.1.0 - 2017-05-20
Copy link
Member

Choose a reason for hiding this comment

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

The version number and date aren't decided yet; If you want to call this section "Unreleased" that'd be helpful :-)

### 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
2 changes: 1 addition & 1 deletion utils/package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "eslint-module-utils",
"version": "2.0.0",
"version": "2.1.0",
Copy link
Member

Choose a reason for hiding this comment

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

please revert this; package.json should never be bumped in a PR - only in a commit directly made to master.

"description": "Core utilities to support eslint-plugin-import and other module-related plugins.",
"engines": {
"node": ">=4"
Expand Down
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