fix: fix the preinstall function logic #55
Conversation
|
This is not the right place to implement this. But before we dive into that, let's first discuss if we even want this, because I'm not sure. On the one hand, per the unix philosophy, output should not include unnecessary information. That a build was not found is unnecessary information because there's a fallback to compiling from source. If that succeeds (and we expect it to) then output should be empty. Also because "no native build was found" can be misinterpreted as an error (#42). If it doesn't succeed, then On the other hand, npm is swallowing the output of install scripts nowadays, so we already get the desired behavior of "silent on success". So we could decide to always be verbose. |
This helps the developers to understand the reasons why their package installation goes through the build process instead of just loading the file. I'll open an issue for npm if
As per my government's rules, error logging must be enabled on all Information Technology (IT) assets throughout the enterprise. So, not sure if the philosophical opinions come into play here. Also, the previous code was executing an unvalidated |
Interesting, but the point is, it's not an error per se.
No, because it's sourced from an npm script. I.e. there's no difference in risk between |
It swallows the output of an install script unless that script exited with a non-zero code. Which I think fits your requirements. |
|
Yeah, so when the build also fails, I can know why the to prebuild was not picked up in the first place. |
bin.js
Outdated
| if (code || !process.argv[3]) process.exit(code) | ||
| exec(process.argv[3]).on('exit', function (code) { | ||
| process.exit(code) | ||
| }) | ||
| if (code) process.exit(code) | ||
| }) | ||
| } | ||
|
|
||
| function preinstall () { | ||
| if (!process.argv[2]) return build() | ||
| exec(process.argv[2]).on('exit', function (code) { | ||
| if (code) process.exit(code) |
There was a problem hiding this comment.
process.argv[2] and process.argv[3] are used here to allow the user to specify commands that should be run before and after the build, respectively. It looks like this PR would currently remove that functionality.
| load() | ||
| } catch (err) { | ||
| // report the error and fall to a build | ||
| console.error(err.message) |
There was a problem hiding this comment.
I don't think this should be an error. That would imply the consuming package has done something wrong or has an unexpected condition to recover from.
More likely in my experience, the package was simply not built.
|
The intent of I would definitely be for making the situation less confusing. Maybe better guidance for |
It simplifies the preinstall logic to call the load function and report errors in case of not finding a prebuild. It then falls back to the build.
Fixes #54
Test result: