Add requireResolve option #1217
Conversation
|
There was no need for a separate PR, for future reference. I'll keep both PRs updated. |
| /*eslint import/no-unresolved: [2, { requireResolve: true }]*/ | ||
| const { default: x } = require.resolve('./foo') // reported if './foo' is not found | ||
|
|
||
| require.resolve(0) // ignored |
There was a problem hiding this comment.
why is this ignored? i'd expect this to be coerced to a string and then validated the same.
There was a problem hiding this comment.
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)There was a problem hiding this comment.
it seems useful to me to have the linter warn on it
There was a problem hiding this comment.
I agree -- but we should follow the same rules we have for commonjs require right?
There was a problem hiding this comment.
Not necessarily - require is used in multiple ways, and so has edge cases - require.resolve, however, does not have those edge cases.
There was a problem hiding this comment.
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()".
|
@benmosher Could you review this when you have a chance? |
|
Hello, any reason why this is not merged? I'd also love to have this functionality. I am also happy to change - can deal with the conflicts and test. Just wouldn't want to do the extra work if there is a separate reason for this not being considered anymore. |
ljharb
left a comment
ljharb
left a comment
There was a problem hiding this comment.
This needs a rebase, and would need test cases as well (and doc updates).
|
@ljharb I'm happy to rebase and do the necessary work. |
|
@lgandecki that'd be great! current tests look OK, but the more the better :-) in general, for every new test case, i'd prefer it to be tested in each parser, and both with and without the new option. |
4 participants
Closes #585
Alternate to #1216
Adds an option
requireResolveto checkrequire.resolvestatements.Might want to support
require.resolveby default in a future major version though.