Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

postinstall incompatible with Windows #212

Closed
binki opened this issue Jul 14, 2017 · 2 comments 路 Fixed by opencomponents/base-templates#128
Closed

postinstall incompatible with Windows #212

binki opened this issue Jul 14, 2017 · 2 comments 路 Fixed by opencomponents/base-templates#128

Comments

@binki
Copy link
Contributor

binki commented Jul 14, 2017

C:\Users\ohnob\repos\lint-staged-play>npm install -S lint-staged

> lint-staged@4.0.1 postinstall C:\Users\ohnob\repos\lint-staged-play\node_modules\lint-staged
> echo "馃毇馃挬 lint-staged installed!
Do not forget to configure it. See https://github.com/okonet/lint-staged/blob/master/README.md" && exit 0

"馃毇馃挬 lint-staged installed!
lint-staged-play@1.0.0 C:\Users\ohnob\repos\lint-staged-play
`-- lint-staged@4.0.1
  +-- app-root-path@2.0.1
  +-- cosmiconfig@1.1.0
  | +-- graceful-fs@4.1.11
  | +-- js-yaml@3.9.0
  | | +-- argparse@1.0.9
  | | | `-- sprintf-js@1.0.3
  | | `-- esprima@4.0.0
  | +-- minimist@1.2.0
  | +-- object-assign@4.1.1
  | +-- os-homedir@1.0.2
  | +-- parse-json@2.2.0
  | | `-- error-ex@1.3.1
  | |   `-- is-arrayish@0.2.1
  | +-- pinkie-promise@2.0.1
  | | `-- pinkie@2.0.4
  | `-- require-from-string@1.2.1
  +-- execa@0.7.0
  | +-- cross-spawn@5.1.0
  | | +-- lru-cache@4.1.1
  | | | +-- pseudomap@1.0.2
  | | | `-- yallist@2.1.2
  | | `-- shebang-command@1.2.0
  | |   `-- shebang-regex@1.0.0
  | +-- get-stream@3.0.0
  | +-- is-stream@1.1.0
  | +-- npm-run-path@2.0.2
  | | `-- path-key@2.0.1
  | +-- p-finally@1.0.0
  | +-- signal-exit@3.0.2
  | `-- strip-eof@1.0.0
  +-- listr@0.12.0
  | +-- chalk@1.1.3
  | | +-- ansi-styles@2.2.1
  | | +-- escape-string-regexp@1.0.5
  | | +-- has-ansi@2.0.0
  | | `-- supports-color@2.0.0
  | +-- cli-truncate@0.2.1
  | | +-- slice-ansi@0.0.4
  | | `-- string-width@1.0.2
  | |   +-- code-point-at@1.1.0
  | |   `-- is-fullwidth-code-point@1.0.0
  | |     `-- number-is-nan@1.0.1
  | +-- figures@1.7.0
  | +-- indent-string@2.1.0
  | | `-- repeating@2.0.1
  | |   `-- is-finite@1.0.2
  | +-- is-promise@2.1.0
  | +-- listr-silent-renderer@1.1.1
  | +-- listr-update-renderer@0.2.0
  | | +-- elegant-spinner@1.0.1
  | | `-- indent-string@3.1.0
  | +-- listr-verbose-renderer@0.4.0
  | | +-- cli-cursor@1.0.2
  | | | `-- restore-cursor@1.0.1
  | | |   +-- exit-hook@1.1.1
  | | |   `-- onetime@1.1.0
  | | `-- date-fns@1.28.5
  | +-- log-symbols@1.0.2
  | +-- log-update@1.0.2
  | | `-- ansi-escapes@1.4.0
  | +-- ora@0.2.3
  | | `-- cli-spinners@0.1.2
  | +-- rxjs@5.4.2
  | | `-- symbol-observable@1.0.4
  | +-- stream-to-observable@0.1.0
  | `-- strip-ansi@3.0.1
  |   `-- ansi-regex@2.1.1
  +-- lodash.chunk@4.2.0
  +-- minimatch@3.0.4
  | `-- brace-expansion@1.1.8
  |   +-- balanced-match@1.0.0
  |   `-- concat-map@0.0.1
  +-- npm-which@3.0.1
  | +-- commander@2.11.0
  | +-- npm-path@2.0.3
  | `-- which@1.2.14
  |   `-- isexe@2.0.0
  +-- p-map@1.1.1
  `-- staged-git-files@0.0.4

npm WARN lint-staged-play@1.0.0 No description
npm WARN lint-staged-play@1.0.0 No repository field.

C:\Users\ohnob\repos\lint-staged-play>

As you can see, cmd鈥檚 ECHO does not treat " as a string delimiter in the same way that sh does. As a result, the " is printed in the output and only the first line is displayed.

Additionally, && exit 0 seems redundant.

If you want to use an echo that behaves consistently cross-platform, should either remove the double-quotes and use two echo commands:

{
  "postinstall": "echo 馃毇馃挬 lint-staged installed! && echo Do not forget to configure it. See https://github.com/okonet/lint-staged/blob/master/README.md"
}
ohnobinki@gibby ~/lint-staged-play $ uname -s
Linux
ohnobinki@gibby ~/lint-staged-play $ npm run postinstall

> lint-staged-play@1.0.0 postinstall /home/ohnobinki/lint-staged-play
> echo 馃毇馃挬 lint-staged installed! && echo Do not forget to configure it. See https://github.com/okonet/lint-staged/blob/master/README.md

馃毇馃挬 lint-staged installed!
Do not forget to configure it. See https://github.com/okonet/lint-staged/blob/master/README.md
C:\Users\ohnob\repos\lint-staged-play>\msys64\usr\bin\uname.exe -s
MSYS_NT-10.0

C:\Users\ohnob\repos\lint-staged-play>npm run postinstall

> lint-staged-play@1.0.0 postinstall C:\Users\ohnob\repos\lint-staged-play
> echo 馃毇馃挬 lint-staged installed! && echo Do not forget to configure it. See https://github.com/okonet/lint-staged/blob/master/README.md

馃毇馃挬 lint-staged installed!
Do not forget to configure it. See https://github.com/okonet/lint-staged/blob/master/README.md

or, if you really want to surround the content of your messages in double-quotes, then use shx鈥檚 echo (note that single-quotes around arguments do not work on Windows):

{
  "postinstall": "shx echo \"馃毇馃挬 lint-staged installed!\" && shx echo \"Do not forget to configure it. See https://github.com/okonet/lint-staged/blob/master/README.md\""
}
ohnobinki@gibby ~/lint-staged-play $ uname -s
Linux
ohnobinki@gibby ~/lint-staged-play $ npm run postinstall

> lint-staged-play@1.0.0 postinstall /home/ohnobinki/lint-staged-play
> shx echo "馃毇馃挬 lint-staged installed!" && shx echo "Do not forget to configure it. See https://github.com/okonet/lint-staged/blob/master/README.md"

馃毇馃挬 lint-staged installed!
Do not forget to configure it. See https://github.com/okonet/lint-staged/blob/master/README.md
C:\Users\ohnob\repos\lint-staged-play>\msys64\usr\bin\uname.exe -s
MSYS_NT-10.0

C:\Users\ohnob\repos\lint-staged-play>npm run postinstall

> lint-staged-play@1.0.0 postinstall C:\Users\ohnob\repos\lint-staged-play
> shx echo "馃毇馃挬 lint-staged installed!" && shx echo "Do not forget to configure it. See https://github.com/okonet/lint-staged/blob/master/README.md"

馃毇馃挬 lint-staged installed!
Do not forget to configure it. See https://github.com/okonet/lint-staged/blob/master/README.md

Double Quotes Unnecessary/Wrong

You might wonder why the exclamation mark (!) doesn鈥檛 cause any problems for sh (unix). That is because ! is only a special character in interactive shells where history expansion is enabled by default. Also, it only causes an error if there is no whitespace after it. So if you use ! that in a terminal, you will get an error. But not in a shellout:

ohnobinki@gibby ~ $ echo "!yo"
bash: !yo: event not found
ohnobinki@gibby ~ $ bash -c 'echo "!yo"'
!yo

Also, double quotes are wrong on Windows because Windows passes the whole command line as a single string. This means that splitting a command鈥檚 parameters into separate arguments and globbing is performed by the invoked application. Which in turn means that different commands implement different string splitting/quoting rules. ECHO literally prints everything passed unless if nothing is passed, in which case ECHO is ON or OFF is printed (to print a blank line, you do ECHO. [sic]).

C:\Users\ohnob\repos\lint-staged-play>ECHO "hi"
"hi"

C:\Users\ohnob\repos\lint-staged-play>ECHO hi
hi

C:\Users\ohnob\repos\lint-staged-play>ECHO
ECHO is on.

C:\Users\ohnob\repos\lint-staged-play>ECHO.


C:\Users\ohnob\repos\lint-staged-play>

Postinstall Pointless

Also, one could argue that if a developer installs this package, they could be expected to look up the documentation on how to use it without prompting. Note that postinstall runs even when the user just types npm install, so the postinstall runs even for projects which already are configured or even when it is an indirect dependency. So is there even a point in a postinstall step that prints stuff out like this? Maybe the best change to improve portability of the package by increasing the similarity of install behavior on different platforms would be to just remove the postinstall script.

binki added a commit to binki/lint-staged that referenced this issue Jul 14, 2017
@okonet
Copy link
Collaborator

okonet commented Jul 17, 2017

Wow that's a detailed bug report. I wish all issues were like this one 馃挴

okonet pushed a commit that referenced this issue Jul 17, 2017
@binki
Copy link
Contributor Author

binki commented Jul 17, 2017

Wow that's a detailed bug report. I wish all issues were like this one 馃挴

It kind of was a rant, sorry xD. Thanks for accepting the simple solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment