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

[New] extensions: accept both a string, and an object to override it. #555

Merged
merged 2 commits into from
Sep 20, 2016
Merged
Show file tree
Hide file tree
Changes from all 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
4 changes: 3 additions & 1 deletion docs/rules/extensions.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,12 @@ In order to provide a consistent use of file extensions across your code base, t

## Rule Details

This rule has one option which could be either a string or an object. If it is `"never"` (the default value) the rule forbids the use for any extension. If `"always"` then the rule enforces the use of extensions for all import statements.
This rule either takes one string option, one object option, or a string and an object option. If it is the string `"never"` (the default value), then the rule forbids the use for any extension. If it is the string `"always"`, then the rule enforces the use of extensions for all import statements.

By providing an object you can configure each extension separately, so for example `{ "js": "always", "json": "never" }` would always enforce the use of the `.js` extension but never allow the use of the `.json` extension.

By providing both a string and an object, the string will set the default setting for all extensions, and the object can be used to set granular overrides for specific extensions. For example, `[<enabled>, "never", { "svg": "always" }]` would require that all extensions are omitted, except for "svg".

### Exception

When disallowing the use of certain extensions this rule makes an exception and allows the use of extension when the file would not be resolvable without extension.
Expand Down
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@
"es6-map": "^0.1.3",
"es6-set": "^0.1.4",
"eslint-import-resolver-node": "^0.2.0",
"has": "^1.0.1",
"lodash.cond": "^4.3.0",
"lodash.endswith": "^4.0.1",
"lodash.find": "^4.3.0",
Expand Down
3 changes: 2 additions & 1 deletion resolvers/webpack/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ var findRoot = require('find-root')
, assign = require('object-assign')
, resolve = require('resolve')
, semver = require('semver')
, has = require('has')

var log = require('debug')('eslint-plugin-import:resolver:webpack')

Expand Down Expand Up @@ -265,7 +266,7 @@ function findExternal(source, externals, context) {

// else, vanilla object
for (var key in externals) {
if (!externals.hasOwnProperty(key)) continue
if (!has(externals, key)) continue
Copy link
Member

Choose a reason for hiding this comment

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

(restarted as an explicit line review, mostly out of curiosity + to gain familiarity with the new feature)

So, I'm not anti-dependencies in general, but I don't understand why

  • someone would have file extensions of toString or hasOwnProperty
  • someone would be surprised if things are broken when they monkeypatch JS core functions

Am I correct that these are the cases where has matters over just a plain ol' hasOwnProperty call or in operator? Because if that's true, I'd still like to revert to hasOwnProperty or really, just extension in modifiers because these are well-understood JS core idioms, and in this specific case, not practically distinguishable.

Copy link
Member

Choose a reason for hiding this comment

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

Looks ready to :shipit:, other than this, though. 😄 Thanks to both of you for working through @jfmengels' feedback, too!

Copy link
Member Author

@ljharb ljharb Sep 17, 2016

Choose a reason for hiding this comment

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

The first one, I also don't know why someone would do it, but given that it's a possibility, that would be a really silly reason for this package to break, don't you think?

As to the second, for a tool like an eslint plugin, that's a stronger case, since it won't be running in browser environments, or environments with unknown third-party code. However, the idea that it's acceptable for one's code to be broken because someone else did something bad, foolish, or simply unexpected mystifies me - as long as my code runs in a pristine environment, nothing that happens afterwards can break it. This seems like a pretty important property to me.

I'll of course remove the dependency if you insist, but I think this is still strictly better because you don't have to think about all these edge cases. I do not think in or hasOwnProperty are well-understood idioms, which is why this problem keeps coming up in libs all over the place, and why has exists.

Copy link
Member Author

@ljharb ljharb Sep 18, 2016

Choose a reason for hiding this comment

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

(@benmosher let me know your final call, and i'll rebase out the last commit if needed)

Copy link
Member

Choose a reason for hiding this comment

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

Fair points. It's not a huge deal either way, or it wouldn't be so easy to have this back and forth. 😄

I hear what you're saying, and maybe one day all our key checks turn into has calls, but I don't think today is that day.

I didn't realize the has part was a separate commit -- I'll just go ahead and merge the first one.

Copy link
Member

Choose a reason for hiding this comment

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

Agh, nevermind, it's bundled with some other stuff. I'll just keep it for now. 😁

if (source === key) return true
}
return false
Expand Down
66 changes: 44 additions & 22 deletions src/rules/extensions.js
Original file line number Diff line number Diff line change
@@ -1,18 +1,27 @@
import path from 'path'
import endsWith from 'lodash.endswith'
import has from 'has'
import assign from 'object-assign'

import resolve from '../core/resolve'
import { isBuiltIn } from '../core/importType'

module.exports = function (context) {
const configuration = context.options[0] || 'never'
const defaultConfig = typeof configuration === 'string' ? configuration : null
const modifiers = assign(
{},
typeof configuration === 'object' ? configuration : context.options[1]
)

function isUseOfExtensionEnforced(extension) {
if (typeof configuration === 'object') {
return configuration[extension] === 'always'
}
function isUseOfExtensionRequired(extension) {
if (!has(modifiers, extension)) { modifiers[extension] = defaultConfig }
return modifiers[extension] === 'always'
}

return configuration === 'always'
function isUseOfExtensionForbidden(extension) {
if (!has(modifiers, extension)) { modifiers[extension] = defaultConfig }
return modifiers[extension] === 'never'
}

function isResolvableWithoutExtension(file) {
Expand All @@ -37,15 +46,15 @@ module.exports = function (context) {
const extension = path.extname(resolvedPath || importPath).substring(1)

if (!extension || !endsWith(importPath, extension)) {
if (isUseOfExtensionEnforced(extension)) {
if (isUseOfExtensionRequired(extension) && !isUseOfExtensionForbidden(extension)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

You could compute the value for the extension modifiers[extension] || defaultConfig once, instead of recomputing it several times (nitpick)

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you just mean, cache it per-extension?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, can-do.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, not cache per-se, though I don't see a reason why not. I was thinking of computing the value of it before line 41, as you will use the result later

Copy link
Member Author

Choose a reason for hiding this comment

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

oh, that's true. let me do that.

Copy link
Member Author

@ljharb ljharb Sep 16, 2016

Choose a reason for hiding this comment

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

This change may not even be worth it overall tho - the tests are failing in Travis because the object is not extensible. Which do you prefer - remove the last commit; rework it to pass with has; or rework it to pass without has?

Copy link
Collaborator

Choose a reason for hiding this comment

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

What are the reasons that externals.hasOwnProperty(key) could break? Is that just when externals is nil?
If you feel like it's better to have it, keep it, I'm not that against it, just didn't see the value if it was just to check if externals[key] exists.

Copy link
Member Author

Choose a reason for hiding this comment

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

so, the reason bracket access and in won't work is because someone could define an extension named "toString" or "hasOwnProperty" or similar. The reason to avoid Object.prototype.hasOwnProperty.call is because that relies on both Function#call and Object#hasOwnProperty being undisturbed in the environment. Using has is robust against both.

Copy link
Member Author

Choose a reason for hiding this comment

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

k, tests should be passing now

Copy link
Member

@benmosher benmosher Sep 17, 2016

Choose a reason for hiding this comment

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

(deleted and moved comment to line in question as a review)

context.report({
node: source,
message:
`Missing file extension ${extension ? `"${extension}" ` : ''}for "${importPath}"`,
})
}
} else if (extension) {
if (!isUseOfExtensionEnforced(extension) && isResolvableWithoutExtension(importPath)) {
if (isUseOfExtensionForbidden(extension) && isResolvableWithoutExtension(importPath)) {
context.report({
node: source,
message: `Unexpected use of file extension "${extension}" for "${importPath}"`,
Expand All @@ -59,18 +68,31 @@ module.exports = function (context) {
}
}

module.exports.schema = [
{
oneOf: [
{
enum: [ 'always', 'never' ],
},
{
type: 'object',
patternProperties: {
'.*': { enum: [ 'always', 'never' ] },
},
},
],
},
]
const enumValues = { enum: [ 'always', 'never' ] }
const patternProperties = {
type: 'object',
patternProperties: { '.*': enumValues },
}

module.exports.schema = {
anyOf: [
{
type: 'array',
items: [enumValues],
additionalItems: false,
},
{
type: 'array',
items: [patternProperties],
additionalItems: false,
},
{
type: 'array',
items: [
enumValues,
patternProperties,
],
additionalItems: false,
},
],
}
53 changes: 53 additions & 0 deletions tests/src/rules/extensions.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,25 @@ ruleTester.run('extensions', rule, {
settings: { 'import/resolve': { 'extensions': [ '.js', '.jsx', '.json' ] } },
}),

test({
code: [
'import lib from "./bar"',
'import lib from "./bar.json"',
'import lib from "./bar.hbs"',
].join('\n'),
options: [ 'always', { js: 'never', jsx: 'never' } ],
settings: { 'import/resolve': { 'extensions': [ '.js', '.jsx', '.json', '.hbs' ] } },
}),

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe add a test case with never, for completeness' sake.
I'd also like to see both valid and invalid test cases with this change, if it's not too much to ask.

test({
code: [
'import lib from "./bar.js"',
'import lib from "./package"',
].join('\n'),
options: [ 'never', { js: 'always', json: 'never' } ],
settings: { 'import/resolve': { 'extensions': [ '.js', '.json' ] } },
}),

// unresolved (#271/#295)
test({ code: 'import path from "path"' }),
test({ code: 'import path from "path"', options: [ 'never' ] }),
Expand Down Expand Up @@ -124,6 +143,40 @@ ruleTester.run('extensions', rule, {
],
}),

test({
code: [
'import lib from "./bar.js"',
'import lib from "./bar.json"',
'import lib from "./bar"',
].join('\n'),
options: [ 'always', { json: 'always', js: 'never', jsx: 'never' } ],
settings: { 'import/resolve': { 'extensions': [ '.js', '.jsx', '.json' ] } },
errors: [
{
message: 'Unexpected use of file extension "js" for "./bar.js"',
line: 1,
column: 17,
},
],
}),

test({
code: [
'import lib from "./bar.js"',
'import lib from "./bar.json"',
'import lib from "./bar"',
].join('\n'),
options: [ 'never', { json: 'always', js: 'never', jsx: 'never' } ],
settings: { 'import/resolve': { 'extensions': [ '.js', '.jsx', '.json' ] } },
errors: [
{
message: 'Unexpected use of file extension "js" for "./bar.js"',
line: 1,
column: 17,
},
],
}),

// unresolved (#271/#295)
test({
code: 'import thing from "./fake-file.js"',
Expand Down