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 support for require.resolve #1216

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

vikr01
Copy link
Contributor

@vikr01 vikr01 commented Oct 20, 2018

Closes #585

Related to discussion in #1035 (link to comment).

So far I've only added require.resolve for commonjs with no-unresolved, let me know if I should add it anywhere else.

@coveralls
Copy link

coveralls commented Oct 20, 2018

Coverage Status

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

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

This is a breaking change, however, unless it’s under an option. Can we make that change?

@vikr01
Copy link
Contributor Author

vikr01 commented Oct 21, 2018

@ljharb was thinking the same thing, yeah I'll make a version using an option

@vikr01
Copy link
Contributor Author

vikr01 commented Oct 24, 2018

Closing in favor of #1217

@vikr01 vikr01 closed this Oct 24, 2018
@ljharb
Copy link
Member

ljharb commented Oct 24, 2018

I'm going to keep the two in sync; orphaned PRs clutter the log.

@ljharb ljharb reopened this Oct 24, 2018
@benmosher
Copy link
Member

@ljharb why is this a breaking change if require.resolve is part of CommonJS?

feels semver minor to me, in that it is sortof a bugfix, but broad enough that it qualifies as an added feature.

I'd prefer this over require-ing (pun intended) users to discover and enable this behavior.

@vikr01
Copy link
Contributor Author

vikr01 commented Oct 25, 2018

@benmosher There may be some who are using require.resolve on files generated at build time (instead of path.join/path.resolve).

@benmosher
Copy link
Member

fair enough, I guess can just try to make the note to bump it to on-by-default in v3.

@ljharb
Copy link
Member

ljharb commented Oct 26, 2018

@benmosher new warnings are almost always a breaking change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Support for checking require.resolve()
4 participants