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 requireResolve option #1217

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft
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
1 change: 1 addition & 0 deletions docs/rules/no-absolute-path.md
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ You may provide an options object providing true/false for any of
- `esmodule`: defaults to `true`
- `commonjs`: defaults to `true`
- `amd`: defaults to `false`
- `requireResolve`: defaults to `false`

If `{ amd: true }` is provided, dependency paths for AMD-style `define` and `require`
calls will be resolved:
Expand Down
10 changes: 10 additions & 0 deletions docs/rules/no-unresolved.md
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,16 @@ define(['./foo'], function (foo) { /*...*/ }) // reported if './foo' is not foun
require(['./foo'], function (foo) { /*...*/ }) // reported if './foo' is not found
```

If `{requireResolve: true}` is provided, single-argument `require.resolve` calls will be resolved:

```js
/*eslint import/no-unresolved: [2, { requireResolve: true }]*/
const { default: x } = require.resolve('./foo') // reported if './foo' is not found

require.resolve(0) // ignored
Copy link
Member

Choose a reason for hiding this comment

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

why is this ignored? i'd expect this to be coerced to a string and then validated the same.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Attempting to run require.resolve(0) gives me this stacktrace:

TypeError [ERR_INVALID_ARG_TYPE]: The "request" argument must be of type string. Received type number
    at Function.resolve (internal/modules/cjs/helpers.js:28:13)

Copy link
Member

Choose a reason for hiding this comment

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

it seems useful to me to have the linter warn on it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree -- but we should follow the same rules we have for commonjs require right?

Copy link
Member

Choose a reason for hiding this comment

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

Not necessarily - require is used in multiple ways, and so has edge cases - require.resolve, however, does not have those edge cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure what you mean. I've never run into any cases where the arguments of require (commonjs) and require.resolve work differently.

From the node documentation it says that require.resolve "gets the exact filename that will be loaded when require()".

require.resolve(['x', 'y'], function (x, y) { /*...*/ }) // ignored
```

#### `ignore`

This rule has its own ignore list, separate from [`import/ignore`]. This is because you may want to know whether a module can be located, regardless of whether it can be parsed for exports: `node_modules`, CoffeeScript files, etc. are all good to resolve properly, but will not be parsed if configured as such via [`import/ignore`].
Expand Down
9 changes: 9 additions & 0 deletions src/rules/no-cycle.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ module.exports = {
const maxDepth = options.maxDepth || Infinity

function checkSourceValue(sourceNode, importer) {
if (isRequireResolve(importer)) return // no require.resolve

const imported = Exports.get(sourceNode.value, context)

if (sourceNode.parent && sourceNode.parent.importKind === 'type') {
Expand Down Expand Up @@ -85,3 +87,10 @@ module.exports = {
function routeString(route) {
return route.map(s => `${s.value}:${s.loc.start.line}`).join('=>')
}

function isRequireResolve(node) {
return (node.type === 'CallExpression'
&& node.callee.type === 'MemberExpression'
&& node.callee.object.name === 'require'
&& node.callee.property.name === 'resolve')
}
11 changes: 11 additions & 0 deletions src/rules/no-useless-path-segments.js
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,17 @@ module.exports = {
type: 'object',
properties: {
commonjs: { type: 'boolean' },
requireResolve: {
oneOf: [
{ type: 'boolean' },
{
type: 'object',
properties: {
commonjs: { type: 'boolean' },
},
},
],
},
},
additionalProperties: false,
},
Expand Down
24 changes: 24 additions & 0 deletions tests/src/rules/no-absolute-path.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,20 @@ ruleTester.run('no-absolute-path', rule, {
test({ code: 'var foo = require("./")'}),
test({ code: 'var foo = require("@scope/foo")'}),

// requireResolve option
test({
code: 'var foo = require.resolve("foo")',
options: [{ requireResolve: { commonjs: true }}],
}),
test({
code: 'var foo = require.resolve("./")',
options: [{ commonjs: true, requireResolve: true }],
}),
test({
code: 'var foo = require.resolve("@scope/foo")',
options: [{ requireResolve: { commonjs: true }}],
}),

test({ code: 'import events from "events"' }),
test({ code: 'import path from "path"' }),
test({ code: 'var events = require("events")' }),
Expand Down Expand Up @@ -86,6 +100,16 @@ ruleTester.run('no-absolute-path', rule, {
options: [{ amd: true }],
errors: [error],
}),
test({
code: 'var f = require.resolve("/some/path")',
options: [{ commonjs: true, requireResolve: true }],
errors: [error],
}),
test({
code: 'var f = require.resolve("/some/path")',
options: [{ requireResolve: { commonjs: true } }],
errors: [error],
}),
// validate amd
test({
code: 'require(["/some/path"], function (f) { /* ... */ })',
Expand Down
4 changes: 4 additions & 0 deletions tests/src/rules/no-cycle.js
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,10 @@ ruleTester.run('no-cycle', rule, {
code: 'import type { FooType } from "./depth-one"',
parser: 'babel-eslint',
}),
test({
code: 'require.resolve("./depth-one")',
options: [{ commonjs: true, requireResolve: true }],
}),
],
invalid: [
test({
Expand Down
31 changes: 26 additions & 5 deletions tests/src/rules/no-relative-parent-imports.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,18 @@ ruleTester.run('no-relative-parent-imports', rule, {
code: 'require("package")',
options: [{ commonjs: true }],
}),
test({
code: 'require.resolve("./internal.js")',
options: [{ commonjs: true, requireResolve: true }],
}),
test({
code: 'require.resolve("./app/index.js")',
options: [{ commonjs: false, requireResolve: { commonjs: true } }],
}),
test({
code: 'require.resolve("package")',
options: [{ requireResolve: { commonjs: true } }],
}),
test({
code: 'import("./internal.js")',
}),
Expand Down Expand Up @@ -70,6 +82,15 @@ ruleTester.run('no-relative-parent-imports', rule, {
column: 9,
} ],
}),
test({
code: 'require.resolve("../plugin.js")',
options: [{ requireResolve: { commonjs: true } }],
errors: [ {
message: 'Relative imports from parent directories are not allowed. Please either pass what you\'re importing through at runtime (dependency injection), move `index.js` to same directory as `../plugin.js` or consider making `../plugin.js` a package.',
line: 1,
column: 17,
} ],
}),
test({
code: 'import("../plugin.js")',
errors: [ {
Expand All @@ -83,16 +104,16 @@ ruleTester.run('no-relative-parent-imports', rule, {
errors: [ {
message: 'Relative imports from parent directories are not allowed. Please either pass what you\'re importing through at runtime (dependency injection), move `index.js` to same directory as `./../plugin.js` or consider making `./../plugin.js` a package.',
line: 1,
column: 17
}]
column: 17,
}],
}),
test({
code: 'import foo from "../../api/service"',
errors: [ {
message: 'Relative imports from parent directories are not allowed. Please either pass what you\'re importing through at runtime (dependency injection), move `index.js` to same directory as `../../api/service` or consider making `../../api/service` a package.',
line: 1,
column: 17
}]
})
column: 17,
}],
}),
],
})
41 changes: 41 additions & 0 deletions tests/src/rules/no-unresolved.js
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,21 @@ function runResolverTests(resolver) {
, options: [{ amd: true }]}),
rest({ code: 'define(["./does-not-exist"], function (bar) {})' }),

// requireResolve setting
rest({ code: 'var foo = require.resolve("./bar")'
, options: [{ requireResolve: true }]}),
rest({ code: 'require.resolve("./bar")'
, options: [{ requireResolve: true }]}),
rest({ code: 'require.resolve("./does-not-exist")'
, options: [{ requireResolve: false }]}),
rest({ code: 'require.resolve("./bar")'
, options: [{ commonjs: false, requireResolve: { commonjs: true } }]}),
rest({ code: 'require.resolve("./does-not-exist")'
, options: [{ requireResolve: { commonjs: false } }]}),
rest({ code: 'require.resolve("./does-not-exist")'
, options: [{ requireResolve: {} }]}),
rest({ code: 'require.resolve("./does-not-exist")' }),

// stress tests
rest({ code: 'require("./does-not-exist", "another arg")'
, options: [{ commonjs: true, amd: true }]}),
Expand Down Expand Up @@ -178,6 +193,32 @@ function runResolverTests(resolver) {
type: 'Literal',
}],
}),

// requireResolve setting
rest({
code: 'var bar = require.resolve("./baz")',
options: [{ commonjs: true, requireResolve: true }],
errors: [{
message: "Unable to resolve path to module './baz'.",
type: 'Literal',
}],
}),
rest({
code: 'require.resolve("./baz")',
options: [{ commonjs: true, requireResolve: true }],
errors: [{
message: "Unable to resolve path to module './baz'.",
type: 'Literal',
}],
}),
rest({
code: 'var bar = require.resolve("./baz")',
options: [{ requireResolve: { commonjs: true } }],
errors: [{
message: "Unable to resolve path to module './baz'.",
type: 'Literal',
}],
}),
],
})

Expand Down
33 changes: 33 additions & 0 deletions tests/src/rules/no-useless-path-segments.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,17 @@ function runResolverTests(resolver) {
// commonjs with default options
test({ code: 'require("./../files/malformed.js")' }),

// requireResolve option
test({
code: 'require("./../files/malformed.js")',
options: [{ commonjs: false, requireResolve: { commonjs: true } }],
}),
test({
code: 'require.resolve("./malformed.js")',
options: [{ commonjs: true, requireResolve: true }],
}),


// esmodule
test({ code: 'import "./malformed.js"' }),
test({ code: 'import "./test-module"' }),
Expand Down Expand Up @@ -62,6 +73,28 @@ function runResolverTests(resolver) {
errors: [ 'Useless path segments for "./deep//a", should be "./deep/a"'],
}),

// requireResolve
test({
code: 'require.resolve("./test-module/")',
options: [{ commonjs: true, requireResolve: true }],
errors: [ 'Useless path segments for "./test-module/", should be "./test-module"'],
}),
test({
code: 'require.resolve("./")',
options: [{ commonjs: true, requireResolve: { commonjs: true } }],
errors: [ 'Useless path segments for "./", should be "."'],
}),
test({
code: 'require.resolve("../")',
options: [{ commonjs: true, requireResolve: true }],
errors: [ 'Useless path segments for "../", should be ".."'],
}),
test({
code: 'require.resolve("./deep//a")',
options: [{ commonjs: false, requireResolve: { commonjs: true } }],
errors: [ 'Useless path segments for "./deep//a", should be "./deep/a"'],
}),

// esmodule
test({
code: 'import "./../files/malformed.js"',
Expand Down
42 changes: 42 additions & 0 deletions utils/moduleVisitor.js
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,19 @@ exports.default = function visitModules(visitor, options) {
}
}

function checkCommonRequireResolve(call) {
if (call.callee.type !== 'MemberExpression') return
if (call.callee.object.name !== 'require') return
if (call.callee.property.name !== 'resolve') return
if (call.arguments.length !== 1) return
vikr01 marked this conversation as resolved.
Show resolved Hide resolved

const modulePath = call.arguments[0]
if (modulePath.type !== 'Literal') return
if (typeof modulePath.value !== 'string') return

checkSourceValue(modulePath, call)
}

const visitors = {}
if (options.esmodule) {
Object.assign(visitors, {
Expand All @@ -97,6 +110,24 @@ exports.default = function visitModules(visitor, options) {
}
}

const requireResolve = {}
if(typeof options.requireResolve === 'boolean') {
Object.assign(requireResolve, {
commonjs: options.commonjs && options.requireResolve,
})
}
else if(options.requireResolve) {
Object.assign(requireResolve, options.requireResolve)
}

if (requireResolve.commonjs) {
const currentCallExpression = visitors['CallExpression']
visitors['CallExpression'] = function (call) {
if (currentCallExpression) currentCallExpression(call)
checkCommonRequireResolve(call)
}
}

return visitors
}

Expand All @@ -111,6 +142,17 @@ function makeOptionsSchema(additionalProperties) {
'commonjs': { 'type': 'boolean' },
'amd': { 'type': 'boolean' },
'esmodule': { 'type': 'boolean' },
'requireResolve': {
'oneOf': [
{ 'type': 'boolean' },
{
'type': 'object',
'properties': {
'commonjs': { 'type': 'boolean' },
vikr01 marked this conversation as resolved.
Show resolved Hide resolved
},
},
],
},
'ignore': {
'type': 'array',
'minItems': 1,
Expand Down