Fix postinstall npm script on Windows

[remix-run]

Fix postinstall npm script on Windows

[bebraw]

Looks like f4623ba introduced a postinstall script. That in turn invokes a Babel based build. This change was needed by #52. The problem is that if you rely on history through git, it won't contain a correct build (generated by prepublish).

I suppose ideally it shouldn't trigger postinstall if you are treating history as a regular dependency. If it's possible to detect how it was installed (from git or in a normal way), then you can skip Babel build altogether (it will point at lib in this case as it should).

[mjackson]

Are you installing from a git repo, @bryaneaton13?

[bryaneaton13]

@mjackson I'm using npm to install it. npm i history -D
It works when I already have babel installed locally or globally, but on a fresh project install (no babel installed anywhere) with history as a dependency, I get this error.

[bebraw]

@mjackson The problem is that postinstall, and hence Babel, gets triggered in a regular install as well for some reason. I can see it has a guard in place

node -e \"require('fs').stat('lib', function (e, s) { process.exit(e || !s.isDirectory() ? 1 : 0) })\"

I suppose the question is why that guard might fail in this case. Why else would it try to trigger Babel unless this check fails?

[bryaneaton13]

I get an error on that line also before I get the babel error message.

[bebraw]

@bryaneaton13 Can you provide more info? Which version of Node are you using? Do you get the same error when running the node -e ... script standalone?

[bryaneaton13]

node -v v0.10.21
npm -v 1.3.11
If I copy and paste that command without || npm run build, it completes.

[taion]

@bryaneaton13 Are you running on Windows?

[bryaneaton13]

@taion Yep

[taion]

I think that's the issue - I believe you can't do cmd1 || cmd2 on Windows.

[mjackson]

I guess that means cmd1 && cmd2 won't work on Windows either, which means our npm test script is probably broken as well. Nice debugging, @taion.

Now that we know what the problem is, anyone want to make a PR?

[taion]

cmd1 && cmd2 actually does work on Windows. It's just the || that doesn't work. ¯_(ツ)_/¯

[mjackson]

ok, @bryaneaton13. I think if we use && instead of || your problem should go away. Can you please try installing from the fix-postinstall-windows branch?

npm install rackt/history#fix-postinstall-windows

[taion]

I think that might break installing when the built files are present, though, since you'd be exiting with a non-zero status code. Per https://docs.npmjs.com/misc/scripts#best-practices:

Don't exit with a non-zero error code unless you really mean it. Except for uninstall scripts, this will cause the npm action to fail, and potentially be rolled back.

[mjackson]

Bah, good point. Any ideas? Last time I wrote code for CMD.exe was at least
a decade ago.

On Thu, Oct 8, 2015 at 5:53 PM Jimmy Jia [email protected] wrote:

I think that might break installing when the built files are present,
though, since you'd be exiting with a non-zero status code. Per
https://docs.npmjs.com/misc/scripts#best-practices:

Don't exit with a non-zero error code unless you really mean it. Except
for uninstall scripts, this will cause the npm action to fail, and
potentially be rolled back.

—
Reply to this email directly or view it on GitHub
#73 (comment).

[taion]

I'm not super familiar with child_process, because I don't script in Node, but this seems to work:

-    "postinstall": "node -e \"require('fs').stat('lib', function (e, s) { process.exit(e || !s.isDirectory() ? 1 : 0) })\" || npm run build"
+    "postinstall": "node -e \"require('fs').stat('lib', function (e, s) { if(e || !s.isDirectory()) require('child_process').spawn('npm', ['run', 'build']) })\""

ETA: I mean it works in Bash, and I think it should work on Windows.

[mjackson]

Ya, I think you're right. Let's try that.