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

Open
wants to merge 5 commits into
base: master
from

Conversation

@vikr01
Copy link
Contributor

vikr01 commented Oct 21, 2018

Closes #585

Alternate to #1216

Adds an option requireResolve to check require.resolve statements.

Might want to support require.resolve by default in a future major version though.

vikr01 added 2 commits Oct 21, 2018
@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 21, 2018

Coverage Status

Coverage increased (+0.09%) to 97.349% when pulling 4773c69 on vikr01:feature/require-resolve-option into b4a2f11 on benmosher:master.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 21, 2018

Coverage Status

Coverage increased (+0.003%) to 97.36% when pulling 1d387cf on vikr01:feature/require-resolve-option into db471a8 on benmosher:master.

@ljharb

This comment has been minimized.

Copy link
Collaborator

ljharb commented Oct 21, 2018

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

This comment has been minimized.

Copy link
@ljharb

ljharb Oct 21, 2018

Collaborator

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

This comment has been minimized.

Copy link
@vikr01

vikr01 Oct 21, 2018

Author Contributor

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)

This comment has been minimized.

Copy link
@ljharb

ljharb Oct 21, 2018

Collaborator

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

This comment has been minimized.

Copy link
@vikr01

vikr01 Oct 22, 2018

Author Contributor

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

This comment has been minimized.

Copy link
@ljharb

ljharb Oct 26, 2018

Collaborator

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

This comment has been minimized.

Copy link
@vikr01

vikr01 Oct 27, 2018

Author Contributor

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()".

utils/moduleVisitor.js Show resolved Hide resolved
vikr01 added 2 commits Oct 30, 2018
@vikr01 vikr01 force-pushed the vikr01:feature/require-resolve-option branch from 1615ae1 to 1d387cf Oct 30, 2018
@vikr01

This comment has been minimized.

Copy link
Contributor Author

vikr01 commented Feb 3, 2019

@benmosher Could you review this when you have a chance?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.