Skip to content
This repository has been archived by the owner on Aug 11, 2022. It is now read-only.

quick fix for possibility that req is null #16937

Merged
merged 1 commit into from
Jun 7, 2017

Conversation

forivall
Copy link
Contributor

@forivall forivall commented Jun 3, 2017

fixes the bug seen in the following log

i have no idea what caused it

or if this is the correct fix

and i want to leave work instead of triaging this

https://gist.github.com/forivall/249c1966dd8ac335bca4a6177e41132d

i used npm shrinkwrap --production. that might be it.

@isaacs isaacs added the review label Jun 3, 2017
@forivall forivall force-pushed the fix-shrinkwrap-production-bug branch from 5804f0e to 1e9f248 Compare June 3, 2017 00:53
@forivall forivall changed the title quick fix for possibility for req to be null quick fix for possibility that req is null Jun 3, 2017
@zkat
Copy link
Contributor

zkat commented Jun 5, 2017

I'm more concerned about why this would be null. I'll leave this PR open for now, but I'm pretty sure we have a bug floating around about this. I'd rather find the root cause.

@zkat zkat changed the base branch from latest to release-next June 7, 2017 23:34
@zkat zkat merged commit 98f739e into npm:release-next Jun 7, 2017
@zkat
Copy link
Contributor

zkat commented Jun 7, 2017

You know what? I'm just gonna take this. Root cause be damned. Thank you, @forivall 👍

@zkat zkat removed the review label Jun 7, 2017
zkat pushed a commit that referenced this pull request Jun 8, 2017
@BridgeAR
Copy link

BridgeAR commented Jun 8, 2017

@zkat doesn't this potentially open the doors for follow up errors and therefore does this really improve the situation?

@zkat
Copy link
Contributor

zkat commented Jun 8, 2017

@BridgeAR it seems to have fixed things in practice, and tbh I'm ok putting a bandaid over it for now: we're going to be taking a closer look at how we deal with trees to clean it up. At the very least, this patch will make the error more obvious, wherever it is.

@stefanpenner
Copy link

stefanpenner commented Jul 7, 2017

@zkat I believe root cause of this problem may be related to:

At the very least, this patch will make the error more obvious, wherever it is.

The bandaid without root cause (or test) seems to be obscuring the problem.

I've left some additional info I've discovered in a recent spelunk: #16994 (comment)

@forivall forivall deleted the fix-shrinkwrap-production-bug branch October 26, 2017 22:12
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants