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

Revert "module: fix inconsistency between load and _findPath" #23228

Closed
wants to merge 1 commit into from

Conversation

@jdalton
Copy link
Member

jdalton commented Oct 2, 2018

This reverts commit 1b92214 to resolve nodejs/citgm#604.
I'll rebase-up making the commit message conform to the rules. I wanted to just get this rolling.

I've been pair programming with @GeoffreyBooth on a follow-up PR to address the inconsistency that #22382 attempted to resolve.

Checklist
@targos

This comment has been minimized.

Copy link
Member

targos commented Oct 2, 2018

I'll rebase-up making the commit message conform to the rules.

It's fine, you don't have to. The rules allow revert messages generated by git

@jasnell
jasnell approved these changes Oct 2, 2018
Copy link
Member

jasnell left a comment

LGTM so long as we have an alternative approach to address the original change here.

@jdalton

This comment has been minimized.

Copy link
Member Author

jdalton commented Oct 3, 2018

@jasnell @targos any ideas on the failing ci/pr run?

@BridgeAR

This comment has been minimized.

Copy link
Member

BridgeAR commented Oct 3, 2018

@jdalton this needs a rebase to pass. That is automatically done on the CI but not on travis.

This reverts commit 1b92214
from PR #22382.

See the discussion at
nodejs/citgm#604

Refs: #22382
Fixes: #4778
@jdalton

This comment has been minimized.

Copy link
Member Author

jdalton commented Oct 3, 2018

@BridgeAR Rebased but no luck.

@BridgeAR

This comment has been minimized.

Copy link
Member

BridgeAR commented Oct 3, 2018

@jdalton the failures are different ones now. I just restarted it to see if the failures are consistent.

@danbev

This comment has been minimized.

Copy link
Member

danbev commented Oct 10, 2018

CI: https://ci.nodejs.org/job/node-test-pull-request/17720/
Sorry if CI was already run but it does not look like just from reading the comments.

@danbev

This comment has been minimized.

Copy link
Member

danbev commented Oct 10, 2018

Landed in b6dcf8c.

@danbev danbev closed this Oct 10, 2018
danbev added a commit that referenced this pull request Oct 10, 2018
This reverts commit 1b92214
from PR #22382.

See the discussion at
nodejs/citgm#604

PR-URL: #23228
Refs: #22382
Fixes: #4778
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@jdalton jdalton deleted the jdalton:revert-22382 branch Oct 10, 2018
@jdalton

This comment has been minimized.

Copy link
Member Author

jdalton commented Oct 10, 2018

Thank you @danbev!

@GeoffreyBooth GeoffreyBooth mentioned this pull request Oct 11, 2018
3 of 4 tasks complete
@targos targos added this to Don't land (ever) in v10.x Oct 16, 2018
jasnell added a commit that referenced this pull request Oct 17, 2018
This reverts commit 1b92214
from PR #22382.

See the discussion at
nodejs/citgm#604

PR-URL: #23228
Refs: #22382
Fixes: #4778
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
v10.x
  
Don't land (ever)
7 participants
You can’t perform that action at this time.