fix: don't emit package-lock.json when package.json does not exist

[xiongmao86]

npm install in a folder that doesn't have package.json doesn't write package-lock.json file.

References

Fixes #6986

[xiongmao86]

Hi, there. It's my first time to contribute to the repo. Thank you very much.

[wraithgar]

Won't this still omit the package-lock if the package.json is an empty object?

[wraithgar]

It seems to me the root problem here is that the check for the existence of a package.json is happening too late. Arborist should never be given the task of reifying if npm is ultimately going to throw an exception.

[xiongmao86]

Thanks very much for the response, @wraithgar, I'll try to figure that out.

[xiongmao86]

Hi, @wraithgar, would you take another look?

[wraithgar]

This would need a test.

[xiongmao86]

Hi, @wraithgar, I have added tests, would you take a look when you have time?

[xiongmao86]

I have fixed the smoke-test proxy test.

[wraithgar]

I took a look and I think the fact that you had to add name to all those fixtures is a signal that this isn't quite right. Unless I'm mistaken @npmcli/package-json will throw an exception by default if you give it a directory with either no package-json file, or one that does not parse correctly. The fact that it is empty shouldn't matter. I suspect that code was left over from when you were using the previous module, which returned an empty object by default.

Another wrinkle here is that I think this check is going to have to happen in Arborist. Consider this situation

npm install abbrev --no-save
mkdir sub
cd sub
npm install

This will generate the same error state, i.e. a package-lock written to the main folder, but this code wouldn't catch it. This is because Arborist does some tree walking to see if any parent directories are in fact the root of the module. The presence of a node_modules folder is one such signal.

What's frustrating is that it appears that wherever is throwing this error is not being properly caught by npm in a way where the error stack is preserved. It appears to be generated without one. After some digging it looks like this is a failure of @npmcli/run-script in the situation where a package-json file is missing.

So. We likely have two bugs here:

First, we have a situation where there is no package.json file, but npm install is called. In that case, Arborist handles things just fine, but when npm goes to try to run the lifecycle scripts it throws an exception. Either run-script needs to more gracefully handle the situation, or npm needs to not call it in this situation.

Second, we have a situation where sometimes Arborist is told to reify a truly empty directory. That is, one with no package lock, package.json, OR node_modules. In that very limited situation it should not be writing the package lock file. We need a much more fine tuned test inside arborist itself to prevent this bug.

Unfortunately when I made this statement

Arborist should never be given the task of reifying if npm is ultimately going to throw an exception.

It was before we knew that there were two bugs. What we should be saying is "Arborist should not write the package-lock on a truly empty set of folders (no package.json, lock, or node_modules)" and "npm should not error when trying to run lifecycle scripts when there is no package.json"

[wraithgar]

Also as a side task I have made npm/run-script#190 and assigned it to myself. We are obviously not done in the process of moving onto that consolidated module for all package json parsing.

[saquibkhan]

why not remove this check, let it generate empty lock file if package.json is empty. Only make sure not to generate lock file if package.json file doesn't exists

[wraithgar]

If you'll read my (admittedly long) comment you'll see that this check in this place is not the right solution to begin with.

[xiongmao86]

Hi, @wraithgar, I have confusion on your comment, would you mind elaborate for me?

I suspect that code was left over from when you were using the previous module, which returned an empty object by default.

I suspect when you mention previous module you mean the rpj call using read-package-json-fast. What is that code which is left over referring to?

What's frustrating is that it appears that wherever is throwing this error is not being properly caught by npm in a way where the error stack is preserved. It appears to be generated without one. After some digging it looks like this is a failure of @npmcli/run-script in the situation where a package-json file is missing.

I have search error stack on google, but I don't understand what it has to do with the current situation? The rpj throw a 'ENOENT` error, why do we need to generate an error stack here?

[wraithgar]

"that code" refers to all the code checking if what was returned was an empty object or not. That's immaterial when using @npmcli/package-json, either it returns a parsed package.json file or it throws an exception.

The generating an error stack comment was about how hard it was to debug. Without one we have no way of knowing where that error is being thrown from. I did discover it however, and it is in fact the run-script call.

[xiongmao86]

Hi, @wraithgar, that code do you mean this snippet? But it's written by me, not left over before I wrote the commit? I still don't understand, Can you point to one place of the code for example?

[wraithgar]

I was trying to infer why the test for an empty object was there and assumed it was due to rpj. The long and the short of it is we don't need that check, either package-json returns or it doesn't, if it returns at all we read a package.json file from somewhere in the tree.

[xiongmao86]

The generating an error stack comment was about how hard it was to debug. Without one we have no way of knowing where that error is being thrown from. I did discover it however, and it is in fact the run-script call.

Doesn't rpj throw an ENOENT error, isn't that enough, @npmcli/package-json would throw an 'ENOENTifpackage.json` is not presented in project dir. Wouldn't they just be the same?