Implement postinstall error handling in a more cross-platform way

[ilkka]

Currently the postinstall script eats errors with a || echo 'ignore' construct. This does not work in PowerShell, and therefore the package is not installable by anybody foolish or brash enough to configure their npm script-shell to be PowerShell. Surely the postinstall script itself could be made to ignore any typical errors by itself? Can somebody shed some light on why it even has that echo at the end?

I realize this may be a bit niche but it'd still be a nice little enhancement not to have to rely on specific shell features that are not universal.

[zloirock]

Because it's more cross-platform than alternatives like

node scripts/postinstall || true

or

node -e \"try { require('./scripts/postinstall') } catch (_) {}\"

Sure it's not perfect, so feel free to propose better sollution for preventing possible errors on postinstall.

[ilkka]

Alright, I see! Do you happen to remember if there were any specific error conditions that this solution was created to solve? The script seems so simple that one would think that it would only fail if the environment was restricted in a very specific way, like having console.log throw if called.

[zloirock]

It's many different errors in many different environments. Some of them you could find in #551 or comments of #548.

[dmwyatt]

Yeah, this broke a ton of my builds.

I'm not sure I agree this is more "cross-platform". This change makes core-js just not work at all on one platform.

[zloirock]

@dmwyatt wdym?

[dmwyatt]

|| is not supported in powershell.

[zloirock]

Feel free to propose a fix.

[dmwyatt]

I don't yet fully grasp the point of the recent change or the point of your postinstall script. However, I do know that this breaks the install for everyone using powershell.

My first reaction is to just get rid of your current postinstall script.

My second reaction is: it worked fine on powershell before the recent change, so revert that change. Is the income stream from developers who sit and read their npm logs that significant?

The Powershell equivalent is something like (node scripts/postinstall.js) -OR (echo "ignore").

I've used run-script-os for doing cross-platform scripts before, though I've never used it for postinstall.

Would you consider a PR that uses that package to solve this issue?

[zloirock]

It is absolutely not related with any income from developers who read logs, it’s just a matter of principle after #548.

it worked fine on powershell before the recent change

Before which change?

The Powershell equivalent is something like...

Unfortunately, this will not work on many platforms.

Would you consider a PR that uses that package to solve this issue?

I'd prefer code without dependencies if it's possible.

[dmwyatt]

Before which change?

The one that introduced the "||" operator.

edit: This one: 39fad35#diff-e47fcc6cb7b1d1082b3a338e00423374

[dopry]

@zloirock just ran into this today. cypress doesn't run under npm scipt-shell cmd.exe so I set to powershell.exe, then core-js wouldn't install.

looking at 39fad35#diff-e47fcc6cb7b1d1082b3a338e00423374

changing the post-install hook to
"postinstall": "node -e \"try { require('./scripts/postinstall'); } catch (e) { }; process.exit(0) \"" might be a possible x-plat fix. what do you think? I'll submit a PR if you need me to.

[zloirock]

@dopry IIRC that causes issues in other platforms. Need to dig out where.

[dopry]

It shouldn't cause issues on any posix compatible platforms and plays nice with powershell and cmd on windows... What other platforms were having an issue? I noticed you changed it from || true to || echo "
ignore". node will universally have process.exit where true might not be present on some platforms. Was true your issue before?

[zloirock]

Here was a reason why I replaced it, maybe you can find it in one of the related issues in the tracker. Maybe someone used npm without node binary in the PATH? I don't remember.

[dopry]

I was able to track down

#551
#557
#559
#556

but none of them seem related to the || true commit. not having node in PATH would be a problem I guess. If you point me in a direction or give me some context for testing I'll look into it further.

[zloirock]

It was a long time ago, I don't remember the source of this issue. Ok, we could try to add it and take a look at what will happen.

[zloirock]

Could you check it with the current version?

[dopry]

@zloirock
Looks resolved... while 3.4.1 generates and error, 3.4.2 installs without error.

PS C:\test> npm i core-js@3.4.2

core-js@3.4.2 postinstall C:\Users\dopry\src\getautomata\test\node_modules\core-js
node -e "try{require('./postinstall')}catch(e){}"

Thank you for using core-js ( https://github.com/zloirock/core-js ) for polyfilling JavaScript standard library!

The project needs your help! Please consider supporting of core-js on Open Collective or Patreon:

https://opencollective.com/core-js
https://www.patreon.com/zloirock

Also, the author of core-js ( https://github.com/zloirock ) is looking for a good job -)

npm notice created a lockfile as package-lock.json. You should commit this file.
npm WARN test@1.0.0 No description
npm WARN test@1.0.0 No repository field.

• core-js@3.4.2
  added 1 package and audited 1 package in 1.004s
  found 0 vulnerabilities

PS C:\test> npm i core-js@3.4.1

core-js@3.4.1 postinstall C:\Users\dopry\src\getautomata\test\node_modules\core-js
node postinstall || echo "ignore"

At line:1 char:18

• node postinstall || echo "ignore"

•              ~~
  

The token '||' is not a valid statement separator in this version.
+ CategoryInfo : ParserError: (:) [], ParentContainsErrorRecordException
+ FullyQualifiedErrorId : InvalidEndOfLine