Update postinstall so that it doesn't cause failures

[mattezell]

There are a few known scenarios where the opencollective postinstall npm script can cause things to break further down the pipeline - a few being 'certain build/CI environments', 'script permission issues' and 'offline installs'....

Arguably, displaying a banner soliciting funding shouldn't disrupt the development or build processes of projects leveraging libraries that are optionally, and non-functionally, using opencollective.

Ultimately, at the heart of the failure, is a non 0 exit code being returned in the cases that opencollective fails to properly execute.

To prevent this non-zero exit code failure so that subsequent npm processes aren't disrupted as a result of non-functional dependency issues, a "|| exit 0" should be added to the postinstall npm script.

There are many discussions about this issue and workaround - here are a few: opencollective/opencollective-cli#5 nuxt/nuxt.js#1357 opencollective/opencollective-cli#3 opencollective/opencollective-postinstall#2 https://github.com/compodoc/compodoc/commit/99ea09f6ac75fe26001c2fae52facc3be1696a52 https://github.com/orizens/ngx-infinite-scroll/pull/321

[WebReflection]

This works already offline and it uses lightercollective.

Can you tell me in which case this would fail?

[WebReflection]

All links and reasons look irrelevant for lightercollective.

Unless there's evidence you have an issue, considering there is an envelope var to skip lightercollective which is like 5 lines of code, I'll close this.

[mattezell]

I disagree with the irrelevant statement - lightercollective, in certain situations, fails essentially identically to the opencollective failures discussed in the provided links, and so the links are therefore relevant. There are multiple discussions on this topic on StackOverflow and in multiple repositories using lightercollective on here.

Here's an issue discussing the topic in relation to lightercollective: https://github.com/webpack/webpack-cli/issues/738

It appears that their solution was to remove lightercollective altogether. Perhaps adding a simple "|| exit 0" so that you can ensure your postinstall doesn't break other things would prevent people from continuing to report lightercollective issues to this repository (I count 6 in here alone).

[WebReflection]

Once again, lightercollective doesn't fail. If it does it's because the user set a logo as path that cannot be reached, but that's a user fault, not a lighter collective one.

However, you have 3 ways to disabled it

• don't include a collective entry in your package.json
• set the rpm config level to silent, error, or warn
• pass DISABLE_OPENCOLLECTIVE=1 upfront (1 or true or y or yes)

TL;DR this is not gonna happen here, that's the wrong place to solve whatever issue you are having.

[WebReflection]

FYI the latest patch has both dependencies updates (no npm audit issues) and latest lightercollective that should not fail.

Please let me know if that's the case, thanks.

[mattezell]

Thanks for your work, Andrea. You seem like a smart person, and it's evident that you're an extremely talented developer. Thanks for forcing me to stretch my grey matter a bit - I've been inside of the guts of npm most of the last day, and definitely feel like I have a MUCH greater understanding of how and why things are being done internally... Please keep in mind that (other) smart and talented people can be wrong or overlook things at times, as it's human - in which case, I personally find that it's often a good idea to step back and consider that perhaps there are aspects that I'm not considering when providing support. Of course that last bit is optional - you're a free person, and so, of course, welcome to support your project in whatever manner you chose.

I'm sharing the following for informational purposes in case someone else stumbles here that this may help... I've gotten to the heart of my specific issue - and I'm betting that a significant amount of people who've reported this in CI or externally managed environments with lightercollective and opencollective most likely have the same peculiarity in their environments.

tl;dr PATH !== Path.

You're correct that lightercollective isn't technically failing - in reality, it's the calling of lightercollective via postinstall that seems to be most people's issue. As mentioned, this may happen for a variety of reasons - some of which these people who've chosen to use your library may be in control of, and some of which they may not (like 3rd party dependencies, 3rd party middleware, host environment configuration, etc,.).

In our API, we have a 3rd party library in play which overrides paths for processes spawned via child_process.spawn() (multi-tenant Enterprise build API). I've now discovered that there were no platform checks being performed (e.g. if process.platform === 'win32', then use process.env['Path'], not process.env['PATH']), and so we were winding up with both env['Path'] and env['PATH'], which are both being passed to child_process.spawn via options - which ultimately make their way to the native Process() instantiation, which in Windows goes with 'Path'.

As you may or may not know, npm-lifecycle is initially responsible for marshaling the various npm lifecycle scripts, such as postinstall, via node's child_process.spawn() (by way of its own spawn wrapper)... During initialization, child_process attempts to infer what casing of 'path' it's dealing with when running in Windows via a case insensitive regex match against each key in process.env - reassigning which version of path casing it will use with each match. So, if multiple env.path are passed in (e.g. 'Path', 'path', 'PATH'), the last one that matches will win out. Before calling spawn(), npm-lifecycle adjusts the env.path using whatever casing of 'path' it ultimately settled on during init to include the library relative node_modules.bin in the env.path that was passed into it by npm when lifecycle() was called (which would here include '......\node_modules\document-register-element\node_modules\.bin', which would include lightercollective/lighercollective.cmd)... But when it ultimately calls child_process.spawn, it passes in the modified env, which includes its modified path instance, as well as any other path casings that may have been passed in... You can walk this chain of passing envPairs to the CPP code wrapping Process.

In my case, I appear to have had env['Path'], and then env['PATH'] coming out of the middleware, and when child_process walked the collection, opting to go with the last case insensitive path regex match, it modified env['PATH'] with the additional paths required to resolve lightercollective (and opencollective) - which was ultimately ignored by Process, when it instead used 'Path' because that's usually the preferred casing in Windows (which is noted via a javascript comment within npm-lifecycle). As mentioned, lifecycle() updated 'PATH', not 'Path', and so the 'Path' that Process() used didn't have the modified paths collection which included the path required to resolve lightercollective. As you've likely guessed, you're not likely to experience this on *nix systems, but it's apparently relatively common on Windows.

Hope this helps someone. I've submitted an issue on this topic to npm-lifecycle along with a pull request, which can be found here: https://github.com/npm/npm-lifecycle/issues/29

I do personally still believe that adding "|| exit 0" is something that all developers relying on external libraries, such as lightercollective and opencollective, should do to ensure that their non-functional request for funding doesn't break functional things of projects which depend on their library as a dependency. Of course, you're free to disagree - bearing in mind that many other widely adopted, well known projects have been doing this very thing (or worse yet, removing the offending dependency altogether). I believe that doing this is not only good for the consumer of your library, but is good for your project as well. I'm an advisory architect for a multinational company of 20K+ employees - and am required to compile approval request forms for every library that we use in our libraries. Part of the report includes risks, such as "may lead to breaking functional things in a project for non-functional requests for money - have raised with author, who's refused to add officially documented solution in similar opencollective library found here https://github.com/nuxt/opencollective", which would likely result in the library landing on the corp denied list... Just food for thought...

Again, thanks for all of your hard work - best of wishes on your projects.

[WebReflection]

Of course, you're free to disagree

It's not that I think my software should cause anyone issues, but I am not a programmer that leaves things to the faith: I want to know, if something fails, why is that.

Turned out, me doing everything I could to be sure my software wouldn't fail in unknown envs/circumstances, became a reason to dig deeper into npm and find out they do have an issue.

As summary: thanks a lot for filing the bug and digging it deeper, I wish the Open Source community would do this more frequently, or out of the box, instead of suggesting || exit 0 everywhere.

Imagine your OS bootstrapping with system calls to software that || exit 0 ... that'd be a joke, not an OS.

We should program more, and joke less 😉

[WebReflection]

P.S. you can fork DRE and change that package script until npm fixes its issue with multi env paths ... DRE is mostly maintenance, so it's not like you'll miss any train. Google AMP did the same for other reasons, Mozilla AFrame has their own version too, this project already demonstrated rock-solid stability and as you can see the amount of issues has been zero for long time.

[mattezell]

As summary: thanks a lot for filing the bug and digging it deeper, I wish the Open Source community would do this more frequently, or out of the box, instead of suggesting || exit 0 everywhere.

Ha! I completely understand and I agree. I'm betting that in 100% of the reported cases of "command not found" issues with both opencollective and lightercollective have some environmental or project issue at the heart of it, rather than it being an issue with either of those two libraries.

This was an interesting exercise for me - I've been using node/npm for more than a decade (and have been a node contributor since v0.7.6 / 2012). This showed me that no matter how much I think I know about node/npm, I only know what I've been forced to learn out of necessity to date - and now, out of necessity, I know a lot more :)

P.S. you can fork DRE and change that package script until npm fixes its issue with multi env paths ...

PR sent.

Thanks again for all of your hard work and contributions to the open source community! Have a great weekend!

[mattezell]

It occurs to me that I may have misunderstood your "fork comment" - feel free to decline/ignore the PR if that wasn't your intent. As mentioned, I have a solution for my situation (and hopefully for others who stumble here) - though it may save some people some headache.

It's interesting... This whole 'thing' got me wondering... "Why have I only seen this issue with opencollective and lightercollective?". I searched our project which currently has 1,100+ top level dependencies. The only npm scripts that I found in those 1,100+ dependencies that made calls to non-bundled scripts were ones that were either calling opencollective or lightercollective...