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.

Sign up for GitHub

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

Jump to bottom

Add requireResolve option #1217

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft

Add requireResolve option #1217

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4773c69
add requireResolve option
vikr01 Oct 21, 2018
e1d0c8e
update docs for requireResolve option
vikr01 Oct 21, 2018
e0deb9e
change requireResolve schema
vikr01 Oct 24, 2018
99096a9
prevent require.resolve from causing no-cycle errors
vikr01 Oct 30, 2018
1d387cf
update test cases using module visitor
vikr01 Oct 30, 2018
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
View file
Open in desktop
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
View file
Open in desktop
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
27 changes: 27 additions & 0 deletions tests/src/rules/no-unresolved.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,15 @@ 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("./does-not-exist")' }),

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

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

Expand Down
22 changes: 22 additions & 0 deletions utils/moduleVisitor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,19 @@ exports.default = function visitModules(visitor, options) {
}
}

function checkRequireResolve(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,14 @@ exports.default = function visitModules(visitor, options) {
}
}

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

return visitors
}

Expand All @@ -111,6 +132,7 @@ function makeOptionsSchema(additionalProperties) {
'commonjs': { 'type': 'boolean' },
'amd': { 'type': 'boolean' },
'esmodule': { 'type': 'boolean' },
'requireResolve': { 'type': 'boolean' },
'ignore': {
'type': 'array',
'minItems': 1,
Expand Down