Fix Windows compatibility #666

GeorgeGorbanev · 2017-01-03T22:22:15Z

'install_generator.rb' uses *nix instruction 'which' to find Node and npm path.

Now it use 'where' if detect running on Windows and 'which' in other case.

This change is

coveralls · 2017-01-03T22:35:36Z

Coverage remained the same at 99.294% when pulling da203cc on GeorgeGorbanev:install_generator_windows_fix into 13cff3f on shakacode:master.

coveralls · 2017-01-03T22:35:36Z

Coverage remained the same at 99.294% when pulling da203cc on GeorgeGorbanev:install_generator_windows_fix into 13cff3f on shakacode:master.

justin808 · 2017-01-05T20:24:00Z

Thanks for this! We need a CHANGELOG.md entry per the instructions in https://github.com/shakacode/react_on_rails/blob/master/CONTRIBUTING.md and need travis to be passing.

Reviewed 1 of 1 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

lib/generators/react_on_rails/install_generator.rb, line 68 at r1 (raw file):

      def missing_npm?
        return false unless
            /cygwin|mswin|mingw|bccwin|wince|emx/ =~ RUBY_PLATFORM ? `where npm`.blank? : `which npm`.blank?

Let's dry up this check in for a windows ruby platform by creating a utility method in https://github.com/shakacode/react_on_rails/blob/master/lib/react_on_rails/utils.rb

Let's add a unit test for this utility method that will compare the result against a different way of figuring out the value, as suggested by the answers here.

http://stackoverflow.com/a/171011/1009332

Comments from Reviewable

justin808 · 2017-01-06T10:12:04Z

@GeorgeGorbanev Why did you close this PR?

GeorgeGorbanev · 2017-01-06T10:19:45Z

@justin808 My bad, it was not expected. Will fix it soon.

GeorgeGorbanev · 2017-01-06T17:00:23Z

@justin808 Added few lines in changelog and separated checking method to utils module. But have some troubles with tests. Practically I never did it before and don't know how to do it. I tried to do it using this tips and on command:

bundle && npm i && rake examples:prepare_all && rake node_package && rake

get error

> [email protected] babel /home/george/RubymineProjects/react_on_rails
> babel --out-dir node_package/lib node_package/src

SyntaxError: node_package/src/Authenticity.js: Unexpected token (6:15)
  4 | 
  5 |   authenticityToken() {
> 6 |     const token: ?HTMLElement = document.querySelector('meta[name="csrf-token"]');
    |                ^
  7 |     if (token && (token instanceof window.HTMLMetaElement)) {
  8 |       return token.content;
  9 |     }

npm ERR! Linux 4.4.0-51-generic
npm ERR! argv "/home/george/.nvm/versions/node/v7.4.0/bin/node" "/home/george/.nvm/versions/node/v7.4.0/bin/npm" "run" "babel"
npm ERR! node v7.4.0
npm ERR! npm  v4.0.5
npm ERR! code ELIFECYCLE
npm ERR! [email protected] babel: `babel --out-dir node_package/lib node_package/src`
npm ERR! Exit status 1

Log file is attached. npm-debug.txt

Can you help me?

justin808 · 2017-01-06T20:19:22Z

@GeorgeGorbanev You'll have to yak shave that one. Possibly there's issues with Windows? Maybe downgrade the version of node to the latest 6.x version?

justin808 · 2017-01-07T17:31:08Z

Please rebase on top of master and try again.

justin808 · 2017-01-08T19:17:47Z

Remove the last commit and please try the rebase again. You should ONLY see the changes that you've made. @GeorgeGorbanev

justin808 · 2017-01-10T06:09:15Z

@GeorgeGorbanev Any progress?

GeorgeGorbanev · 2017-01-10T07:12:55Z

@justin808 Yes, some kind of progress. I configured system to run tests and ready to write rspec. Also, did I rebased branch right way this time?

justin808 · 2017-01-10T07:26:10Z

almost there. Yes, tests and pass CI. did you lint?

Reviewed 2 of 3 files at r2, 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks broke.

Comments from Reviewable

GeorgeGorbanev · 2017-01-10T07:34:48Z

@justin808 Guess no, don't know how to do it.

justin808 · 2017-01-10T07:40:27Z

https://github.com/shakacode/react_on_rails/blob/master/CONTRIBUTING.md

rubocop probably

GeorgeGorbanev · 2017-01-10T07:53:57Z

@justin808 Thank you. It will be done.

coveralls · 2017-01-10T13:39:18Z

Coverage increased (+0.001%) to 99.299% when pulling b368777 on GeorgeGorbanev:install_generator_windows_fix into 6bb07f5 on shakacode:master.

coveralls · 2017-01-10T20:39:22Z

Coverage increased (+0.001%) to 99.299% when pulling b1b247f on GeorgeGorbanev:install_generator_windows_fix into a607ad7 on shakacode:master.

coveralls · 2017-01-10T20:39:22Z

Coverage increased (+0.001%) to 99.299% when pulling b1b247f on GeorgeGorbanev:install_generator_windows_fix into a607ad7 on shakacode:master.

coveralls · 2017-01-10T21:19:29Z

Coverage increased (+0.001%) to 99.299% when pulling 2085aa2 on GeorgeGorbanev:install_generator_windows_fix into a607ad7 on shakacode:master.

coveralls · 2017-01-10T21:19:29Z

Coverage increased (+0.001%) to 99.299% when pulling 2085aa2 on GeorgeGorbanev:install_generator_windows_fix into a607ad7 on shakacode:master.

coveralls · 2017-01-10T21:19:29Z

Coverage increased (+0.001%) to 99.299% when pulling 2085aa2 on GeorgeGorbanev:install_generator_windows_fix into a607ad7 on shakacode:master.

coveralls · 2017-01-10T21:28:14Z

Coverage increased (+0.001%) to 99.299% when pulling 2085aa2 on GeorgeGorbanev:install_generator_windows_fix into a607ad7 on shakacode:master.

GeorgeGorbanev · 2017-01-10T21:42:50Z

@justin808 How it looks now?

justin808 · 2017-01-12T23:38:05Z

some more comments

Reviewed 5 of 5 files at r5.
Review status: 3 of 4 files reviewed at latest revision, 6 unresolved discussions.

CHANGELOG.md, line 19 at r5 (raw file):

### Fixed
- Removed foreman as a dependency. [#678](https://github.com/shakacode/react_on_rails/pull/678) by [x2es](https://github.com/x2es).
- Added checking of OS and fixed generator for Windows.

Please double check that your formatting matches all other entries.

get rid of the ====== and add the PR number and your github info!

lib/react_on_rails/utils.rb, line 43 at r5 (raw file):

      def self.linux?
        OS.unix? && !OS.mac?

Why do we care if mac or unix?

spec/react_on_rails/generators/install_generator_spec.rb, line 168 at r5 (raw file):

def `(*)
what is this syntax?

Comments from Reviewable

GeorgeGorbanev · 2017-01-12T23:57:53Z

@justin808 Thanks for review and sorry for really dirty code. I will follow all your comments.
It would be right to use this method to detect OS?
And can I keep "def `(where)..." to mock windows system calls? Lint will not correctly take it.

justin808 · 2017-01-12T23:59:34Z

I've never seen the def `(where) syntax

I don't see why you will call the wrong command on the wrong OS. That's the point of your fix.

And yes, no need to detect Mac.

GeorgeGorbanev · 2017-01-13T00:20:11Z

@justin808 This syntax described by Hal Fulton for example. Hope it is not bad style.

I don't see why you will call the wrong command on the wrong OS. That's the point of your fix.

Actually it turns wrong command to correct, so we can test method behavior as it would be correct OS. Now it defined in wrong place and "node_missing?" can't use it, but I'll fix it, when find how. Better do another way?

GeorgeGorbanev · 2017-01-13T00:47:13Z

@justin808 Besides a RUBY_PLATFORM constant method I found method using shell command "uname -s" and method using rbconfig. But all of them require the same regexp-matching. Did you mean one of them?
Also, is it necessary to make new spec for this utility-method when it now tests with "missing_node?"?(it still not right, but soon will be)

coveralls · 2017-01-13T12:14:23Z

Coverage increased (+0.008%) to 99.329% when pulling 182f65e on GeorgeGorbanev:install_generator_windows_fix into 9f0f63b on shakacode:master.

coveralls · 2017-01-13T12:48:22Z

Coverage increased (+0.008%) to 99.329% when pulling 267e753 on GeorgeGorbanev:install_generator_windows_fix into dcd2a92 on shakacode:master.

coveralls · 2017-01-13T12:48:22Z

Coverage increased (+0.008%) to 99.329% when pulling 267e753 on GeorgeGorbanev:install_generator_windows_fix into dcd2a92 on shakacode:master.

coveralls · 2017-01-13T12:48:22Z

Coverage increased (+0.008%) to 99.329% when pulling 267e753 on GeorgeGorbanev:install_generator_windows_fix into dcd2a92 on shakacode:master.

coveralls · 2017-01-13T13:15:59Z

Coverage increased (+0.008%) to 99.329% when pulling 67a7f02 on GeorgeGorbanev:install_generator_windows_fix into dcd2a92 on shakacode:master.

coveralls · 2017-01-13T13:15:59Z

Coverage increased (+0.008%) to 99.329% when pulling 67a7f02 on GeorgeGorbanev:install_generator_windows_fix into dcd2a92 on shakacode:master.

coveralls · 2017-01-13T13:15:59Z

Coverage increased (+0.008%) to 99.329% when pulling 67a7f02 on GeorgeGorbanev:install_generator_windows_fix into dcd2a92 on shakacode:master.

coveralls · 2017-01-13T13:15:59Z

Coverage increased (+0.008%) to 99.329% when pulling 67a7f02 on GeorgeGorbanev:install_generator_windows_fix into dcd2a92 on shakacode:master.

coveralls · 2017-01-13T13:34:14Z

Coverage increased (+0.008%) to 99.329% when pulling 13cff3f on GeorgeGorbanev:install_generator_windows_fix into dcd2a92 on shakacode:master.

… unit-tests for it.

coveralls · 2017-01-13T13:43:52Z