Fixed bundle-phobia-install not working

[soimon]

The bundle-phobia-install command didn't work for me, regardless of #37.
Every attempt failed with the error npm install returned with status code undefined
To fix it, I encapsulated the actual execFile within const performInstall with a Promise.

Reasons to believe that should be the case:

1. Every invocation of the function is awaited
2. The same function in npm-utils.js returns a Promise
3. execFile is asynchronous, so acting on the direct result instead of in a callback does not guarantee the process has been finished
4. The current error check relies on res.code where res is the result of execFile. The resulting ChildProcess does in fact not contain a property called code (hence the "status code undefined")

The reason this has not been identified in integration tests is that the test in question mocks the execFile (incorrectly: it contains a property code which the documented ChildProcess doesn't), so it never has to deal with a process error. I would propose a fix for this as well, but I do not feel confident enough in your testing stack so I'll leave that decision to the author 🤕🤷‍♂️

(If all is well you should be able to commit to my branch)

[codecov]

Codecov Report

Merging #40 (6c0286c) into master (f37949a) will not change coverage.
The diff coverage is 0.00%.

@@           Coverage Diff           @@
##           master      #40   +/-   ##
=======================================
  Coverage   33.78%   33.78%           
=======================================
  Files           8        8           
  Lines         293      293           
=======================================
  Hits           99       99           
  Misses        194      194           

Impacted Files	Coverage Δ
src/install.js	0.00% <0.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f37949a...6c0286c. Read the comment docs.

[soimon]

I actually updated fakeExecFile to behave in line with the documentation. I also added a stdout and stderror parameter to the factory method to pave the way for mocking the npm service (that way you could actually test the error response, for example in npm-utils.js in the getVersionList function).

The branch as proposed functions properly (at least on my machine). Because I don't use the feature myself this will probably be my last commit on the branch, I hope it's of some use 😄

[AdrieanKhisbe]

Thanks a lot for the detailed update @soimon, will try to review it quickly.

(seems like I might have overlooked too much things in #33, and forgot that I was mocking too much stuff. 🤦)

note to self 🔖: Might be interesting at some point to add real end-to-end test with no code mocking

[AdrieanKhisbe]

note to self 🔖: Might be interesting at some point to add real end-to-end test with no code mocking

in fact it might be the time, would be more confident with e2e test with no code mocking
Will try on a subbranch

[soimon]

New proposal: if you define the purpose of this command as "conditionally run npm install", then it would make sense to just spawn npm install if it passes the size requirement and pipe the output of that command to the user. The npm log has plenty of (valuable) information about the process, success or failure so there should be no need to provide your own custom error handling and messaging. Like this the command becomes a seamless drop-in for npm install.

The commit is a bit large because of renaming all execFile to spawn in the integration tests for the sake of consistency. Functionally these changes do not have any effect so you can just merge install.js if you wish to rework the integration tests anyway. 💪

[AdrieanKhisbe]

@soimon That's a very good idea indeed, and a clear improvement!

On my side I worked on the install safety net (cf install-fix-and-e2e), and adapted it to your latest changes, it's green as you can see

(I'll open a dedicated PR for extra e2e once this one lands.)

[AdrieanKhisbe]

Thanks a lot @soimon for this great contributions.

I just squashed it, and open a PR with rebased tests I was pointing.
Will publish a patch once I'll have merged the #41 and updated the changelog accordingly.

[soimon]

Great! No problem, nice to finally get the gist of GitHub 😉
I look forwards to the changes and thanks for the great package.

[AdrieanKhisbe]

and here it is @soimon 😉
bundle-phobia-cli@0.14.12 is out. 🚀

🙂