module: don't search in require.resolve.paths by cjihrig · Pull Request #23683 · nodejs/node (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation19 Commits1 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

cjihrig

The paths used by require.resolve() should be treated as starting points for module resolution, and not actually searched. This change aligns the docs with the actual behavior.

Fixes: #18408
Refs: #23643

Checklist

@cjihrig cjihrig added the semver-major

PRs that contain breaking changes and should be released in the next major version.

label

Oct 16, 2018

@nodejs-github-bot

jasnell

@jasnell

Definitely will need a citgm run.

@jdalton

For reference this #18408 (comment) explained the issue nicely.

Update:

The issue #18408 mentioned

This bug was introduced in v9.3.0. v9.2.1 and prior behave correctly.

Do we know what was changed/introduced then?

Update:

This related #17113 was introduced in 9.3.0.

@Trott

@nodejs/tsc This needs a second TSC approver.

@cjihrig

@Trott

targos

@targos

mcollina

Choose a reason for hiding this comment

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

LGTM

@Trott

@Trott

@Trott

@Trott

Needs a CITGM run, I think?

@Trott

@cjihrig

@richardlau

@cjihrig

@richardlau it looks like that job finished with the same top level red and green combination as before. Are the failures expected?

@richardlau

Of the failures https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1677/#showFailuresLink

Test Result (8 failures / +8)

    citgm.stylus-v0.54.5
    citgm.graceful-fs-v4.1.15
    citgm.stylus-v0.54.5
    citgm.graceful-fs-v4.1.15
    citgm.bluebird-v3.5.3
    citgm.stylus-v0.54.5
    citgm.graceful-fs-v4.1.15
    citgm.node-report-v2.2.1

I believe stylus failures are expected (nodejs/citgm#616 (comment), cc @targos I presume the followup to skip stylus didn't happen). I'll look into the node-report failure, but it seems unlikely to be this PR.

No idea if bluebird and graceful-fs have been failing on other recent citgm runs.

@targos

I've already seen graceful-fs and bluebird fail recently

@cjihrig

Thanks @richardlau and @targos! I'm going to land this later tonight or tomorrow unless someone links this PR to the failures (or lands it before I do).

@cjihrig

The paths used by require.resolve() should be treated as starting points for module resolution, and not actually searched.

PR-URL: nodejs#23683 Fixes: nodejs#18408 Refs: nodejs#23643 Reviewed-By: James M Snell jasnell@gmail.com Reviewed-By: Michaël Zasso targos@protonmail.com Reviewed-By: Matteo Collina matteo.collina@gmail.com

refack pushed a commit to refack/node that referenced this pull request

Jan 14, 2019

@cjihrig @refack

The paths used by require.resolve() should be treated as starting points for module resolution, and not actually searched.

PR-URL: nodejs#23683 Fixes: nodejs#18408 Refs: nodejs#23643 Reviewed-By: James M Snell jasnell@gmail.com Reviewed-By: Michaël Zasso targos@protonmail.com Reviewed-By: Matteo Collina matteo.collina@gmail.com

BethGriggs added a commit that referenced this pull request

Apr 22, 2019

@BethGriggs

Notable changes:

PR-URL: #26930

BethGriggs added a commit that referenced this pull request

Apr 23, 2019

@BethGriggs

Notable changes:

PR-URL: #26930

BethGriggs added a commit that referenced this pull request

Apr 23, 2019

@BethGriggs

Notable changes:

PR-URL: #26930

This was referenced

Apr 23, 2019

@Trott Trott mentioned this pull request

May 6, 2019

Labels

semver-major

PRs that contain breaking changes and should be released in the next major version.