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

Add no-absolute-path rule (fixes #530) #538

Merged
merged 2 commits into from
Sep 1, 2016
Merged
Show file tree
Hide file tree
Changes from 1 commit
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: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ This change log adheres to standards from [Keep a CHANGELOG](http://keepachangel
## [Unreleased]
### Added
- Added an `allow` option to [`no-nodejs-modules`] to allow exceptions ([#452], [#509]).
- Added [`no-absolute-path`] rule ([#530], [#538])

### Fixed
- [`no-named-as-default-member`] Allow default import to have a property named "default" ([#507]+[#508], thanks [@jquense] for both!)
Expand Down Expand Up @@ -293,7 +294,9 @@ for info on changes for earlier releases.
[`no-mutable-exports`]: ./docs/rules/no-mutable-exports.md
[`prefer-default-export`]: ./docs/rules/prefer-default-export.md
[`no-restricted-paths`]: ./docs/rules/no-restricted-paths.md
[`no-absolute-path`]: ./docs/rules/no-absolute-path.md

[#538]: https://github.com/benmosher/eslint-plugin-import/pull/538
[#509]: https://github.com/benmosher/eslint-plugin-import/pull/509
[#508]: https://github.com/benmosher/eslint-plugin-import/pull/508
[#503]: https://github.com/benmosher/eslint-plugin-import/pull/503
Expand Down Expand Up @@ -334,6 +337,7 @@ for info on changes for earlier releases.
[#157]: https://github.com/benmosher/eslint-plugin-import/pull/157
[#314]: https://github.com/benmosher/eslint-plugin-import/pull/314

[#530]: https://github.com/benmosher/eslint-plugin-import/issues/530
[#507]: https://github.com/benmosher/eslint-plugin-import/issues/507
[#478]: https://github.com/benmosher/eslint-plugin-import/issues/478
[#456]: https://github.com/benmosher/eslint-plugin-import/issues/456
Expand Down
2 changes: 2 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,14 @@ This plugin intends to support linting of ES2015+ (ES6+) import/export syntax, a
* Ensure a default export is present, given a default import. ([`default`])
* Ensure imported namespaces contain dereferenced properties as they are dereferenced. ([`namespace`])
* Restrict which files can be imported in a given folder ([`no-restricted-paths`])
* Forbid import of modules using absolute paths ([`no-absolute-path`])

[`no-unresolved`]: ./docs/rules/no-unresolved.md
[`named`]: ./docs/rules/named.md
[`default`]: ./docs/rules/default.md
[`namespace`]: ./docs/rules/namespace.md
[`no-restricted-paths`]: ./docs/rules/no-restricted-paths.md
[`no-absolute-path`]: ./docs/rules/no-absolute-path.md

**Helpful warnings:**

Expand Down
27 changes: 27 additions & 0 deletions docs/rules/no-absolute-path.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
# Forbid import of modules using absolute paths

Node.js allows the import of modules using an absolute path such as `/home/xyz/file.js`. That is a bad practice as it ties the code using it to your computer, and therefore makes it unusable in packages distributed on `npm` for instance.

## Rule Details

### Fail

```js
import f from '/foo';
import f from '/some/path';

var f = require('/foo');
var f = require('/some/path');
```

### Pass

```js
import _ from 'lodash';
import foo from 'foo';
import foo from './foo';

var _ = require('lodash');
var foo = require('foo');
var foo = require('./foo');
```
5 changes: 5 additions & 0 deletions src/core/importType.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,10 @@ function constant(value) {
return () => value
}

function isAbsolute(name) {
return name.indexOf('/') === 0
}

export function isBuiltIn(name, settings) {
const extras = (settings && settings['import/core-modules']) || []
return builtinModules.indexOf(name) !== -1 || extras.indexOf(name) > -1
Expand Down Expand Up @@ -46,6 +50,7 @@ function isRelativeToSibling(name) {
}

const typeTest = cond([
[isAbsolute, constant('absolute')],
[isBuiltIn, constant('builtin')],
[isExternalModule, constant('external')],
[isScoped, constant('external')],
Expand Down
1 change: 1 addition & 0 deletions src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ export const rules = {
'no-duplicates': require('./rules/no-duplicates'),
'imports-first': require('./rules/imports-first'),
'no-extraneous-dependencies': require('./rules/no-extraneous-dependencies'),
'no-absolute-path': require('./rules/no-absolute-path'),
'no-nodejs-modules': require('./rules/no-nodejs-modules'),
'order': require('./rules/order'),
'newline-after-import': require('./rules/newline-after-import'),
Expand Down
21 changes: 21 additions & 0 deletions src/rules/no-absolute-path.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
import importType from '../core/importType'
import isStaticRequire from '../core/staticRequire'

function reportIfMissing(context, node, name) {
if (importType(name, context) === 'absolute') {
context.report(node, 'Do not import modules using an absolute path')
}
}

module.exports = function (context) {
return {
ImportDeclaration: function handleImports(node) {
reportIfMissing(context, node, node.source.value)
},
CallExpression: function handleRequires(node) {
if (isStaticRequire(node)) {
reportIfMissing(context, node, node.arguments[0].value)
}
},
}
}
11 changes: 9 additions & 2 deletions tests/src/core/importType.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,12 @@ import { testContext } from '../utils'
describe('importType(name)', function () {
const context = testContext()

it("should return 'absolute' for node.js modules", function() {
Copy link
Member

Choose a reason for hiding this comment

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

What does 'node.js' mean here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That means typo 😅

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed

expect(importType('/', context)).to.equal('absolute')
expect(importType('/path', context)).to.equal('absolute')
expect(importType('/some/path', context)).to.equal('absolute')
})

it("should return 'builtin' for node.js modules", function() {
expect(importType('fs', context)).to.equal('builtin')
expect(importType('path', context)).to.equal('builtin')
Expand Down Expand Up @@ -47,15 +53,16 @@ describe('importType(name)', function () {
expect(importType('./importType/index.js', context)).to.equal('sibling')
})

describe("should return 'index' for sibling index file", function() {
it("should return 'index' for sibling index file", function() {
expect(importType('.', context)).to.equal('index')
expect(importType('./', context)).to.equal('index')
expect(importType('./index', context)).to.equal('index')
expect(importType('./index.js', context)).to.equal('index')
})

it("should return 'unknown' for any unhandled cases", function() {
expect(importType('/malformed', context)).to.equal('unknown')
expect(importType('@malformed', context)).to.equal('unknown')
expect(importType(' /malformed', context)).to.equal('unknown')
expect(importType(' foo', context)).to.equal('unknown')
})

Expand Down
86 changes: 86 additions & 0 deletions tests/src/rules/no-absolute-path.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
import { test } from '../utils'

import { RuleTester } from 'eslint'

const ruleTester = new RuleTester()
, rule = require('rules/no-absolute-path')

const error = {
ruleId: 'no-absolute-path',
message: 'Do not import modules using an absolute path',
}

ruleTester.run('no-absolute-path', rule, {
valid: [
test({ code: 'import _ from "lodash"'}),
test({ code: 'import find from "lodash.find"'}),
test({ code: 'import foo from "./foo"'}),
test({ code: 'import foo from "../foo"'}),
test({ code: 'import foo from "foo"'}),
test({ code: 'import foo from "./"'}),
test({ code: 'import foo from "@scope/foo"'}),
test({ code: 'var _ = require("lodash")'}),
test({ code: 'var find = require("lodash.find")'}),
test({ code: 'var foo = require("./foo")'}),
test({ code: 'var foo = require("../foo")'}),
test({ code: 'var foo = require("foo")'}),
test({ code: 'var foo = require("./")'}),
test({ code: 'var foo = require("@scope/foo")'}),
test({
code: 'import events from "events"',
options: [{
allow: ['events'],
}],
}),
test({
code: 'import path from "path"',
options: [{
allow: ['path'],
}],
}),
test({
code: 'var events = require("events")',
options: [{
allow: ['events'],
}],
}),
test({
code: 'var path = require("path")',
options: [{
allow: ['path'],
}],
}),
test({
code: 'import path from "path";import events from "events"',
options: [{
allow: ['path', 'events'],
}],
}),
],
invalid: [
test({
code: 'import f from "/foo"',
errors: [error],
}),
test({
code: 'import f from "/foo/path"',
errors: [error],
}),
test({
code: 'import f from "/some/path"',
errors: [error],
}),
test({
code: 'var f = require("/foo")',
errors: [error],
}),
test({
code: 'var f = require("/foo/path")',
errors: [error],
}),
test({
code: 'var f = require("/some/path")',
errors: [error],
}),
],
})