Basic yarn support

[jd20]

I was running into #42 and downgrading NPM didn't seem like a viable option for me, so I decided to add basic support for yarn to rn-nodeify. Not sure if this is sufficient, I'm very new to JavaScript and submitting PR's, so comments are welcome (or feel free to disregard if this isn't useful).

To use, simply add --yarn when invoking the rn-nodeify command. I tested with yarn 0.21.3 on OSX, and everything seemed to work ok.

[mvayngrib]

@jd20 thanks, though I noticed an infinite loop!

It's partly due to yarn running "postinstall" after any dependency being installed, vs npm running it only after a bare npm install. To reproduce, check out: https://github.com/mvayngrib/rnctest1/tree/yarn (see the log below)

To fix this, you might want to add support for checking against existing deps here (for yarn vs npm):

https://github.com/monolithlabs/rn-nodeify/blob/yarn/cmd.js#L118

yarn install v0.27.5
[1/4] Resolving packages...
[2/4] Fetching packages...
[3/4] Linking dependencies...
warning "react-native-level-fs@3.0.0" has unmet peer dependency "asyncstorage-down@^3.0.0".
[4/4] Building fresh packages...
$ npm run nodeify

> rnctest1@0.0.1 nodeify /Users/tenaciousmv/Code/rnctest1
> rn-nodeify --install --hack --overwrite --yarn

not reinstalling assert
not reinstalling browserify-zlib
not reinstalling buffer
not reinstalling inherits
not reinstalling console-browserify
not reinstalling constants-browserify
not reinstalling react-native-crypto
not reinstalling dns.js
not reinstalling domain-browser
not reinstalling events
not reinstalling https-browserify
not reinstalling react-native-os
not reinstalling path-browserify
not reinstalling process
not reinstalling punycode
not reinstalling querystring-es3
not reinstalling react-native-level-fs
not reinstalling react-native-udp
not reinstalling stream-browserify
not reinstalling string_decoder
not reinstalling timers-browserify
not reinstalling tty-browserify
not reinstalling url
not reinstalling util
not reinstalling react-native-tcp
not reinstalling vm-browserify
not reinstalling readable-stream
not reinstalling react-native-randombytes
installing from github react-native-http
installing: yarn add tradle/react-native-http#834492d 
yarn add v0.27.5
[1/4] Resolving packages...
[2/4] Fetching packages...
[3/4] Linking dependencies...
warning "react-native-level-fs@3.0.0" has unmet peer dependency "asyncstorage-down@^3.0.0".
[4/4] Building fresh packages...
success Saved 1 new dependency.
└─ react-native-http@1.0.0
$ npm run nodeify

> rnctest1@0.0.1 nodeify /Users/tenaciousmv/Code/rnctest1
> rn-nodeify --install --hack --overwrite --yarn

not reinstalling assert
not reinstalling browserify-zlib
not reinstalling buffer
not reinstalling inherits
not reinstalling constants-browserify
not reinstalling console-browserify
not reinstalling react-native-crypto
not reinstalling dns.js
not reinstalling domain-browser
not reinstalling events
not reinstalling https-browserify
not reinstalling react-native-os
not reinstalling path-browserify
not reinstalling process
not reinstalling punycode
not reinstalling querystring-es3
not reinstalling react-native-level-fs
not reinstalling react-native-udp
not reinstalling stream-browserify
not reinstalling string_decoder
not reinstalling timers-browserify
not reinstalling tty-browserify
not reinstalling url
not reinstalling util
not reinstalling react-native-tcp
not reinstalling vm-browserify
not reinstalling react-native-randombytes
not reinstalling readable-stream
installing from github react-native-http
installing: yarn add tradle/react-native-http#834492d 

[jd20]

Cool, thanks for spotting that, didn't notice that when I ran it locally. I'll try to repro from your branch, and put in a fix as you suggested.

[jd20]

So, digging a bit deeper, seems I didn't notice the infinite loop because I didn't use --overwrite in my testing. With --overwrite, we'll attempt to reinstall a package if we think the version maybe out of date. The particular package causing issues was react-native-http, because there's no trivial way to compare the version of "tradle/react-native-http#834492d" (what is requested in shims.js) and "1.0.0", the package version corresponding to that commit. So, our semver compare will fail, and we'll always end up running "yarn add", causing "postinstall" to run again.

I thought of three ways to fix this: 1) We could try to fetch the package.json from github, and resolve the package version from the commit id we have, but this seems overly complex and brittle. 2) Use --ignore-scripts, to prevent rn-nodeify from being run again. But this causes both top-level and dependency-level scripts not to be run, which may cause other important scripts not to be run, and break things. 3) When calling yarn from rn-nodeify, use a flag (such as an env var) to signal that rn-nodeify shouldn't be spawned recursively. This will allow other important scripts to be run, and only prevents rn-nodeify from being re-spawned.

I ended up going the last approach, I made it yarn-specific, just to limit any side effects I'm not foreseeing. LMK what you think.

[mvayngrib]

@jd20 cool! The last approach didn't occur to me. The main downside I see to it is that, if i understand correctly, react-native-http will now we installed once per postinstall. Once is better than infinity, but still. Depending on when yarn.lock gets updated in the install/postinstall cycle, can we parse that and compare the git hashes?

[mvayngrib]

maybe a combination of the two? Parse yarn.lock but also set the environment variable? The updated lockfile might not be available the first time, but should be on any subsequent installs.

[jd20]

Yeah, that's what I'm thinking, a combination might be needed. I was looking at parse-yarn-lock, but there's a lot of versions, and I'm not sure which one will work for all users of rn-nodeify. Maybe just trying to parse it as a YAML file and silently ignore if we fail to parse, would be best?

The other thing I'd maybe be worried about, is when we call yarn to install http-react-native, I think it's doing more to check the actual integrity of the installed module, than just comparing version from yarn.lock to package.json. So, by doing our own comparison, we may save a bit of extra cpu work, but open up users to more weird behavior if they muck around with any of the files directly. Most users are not using --overwrite I assume too, so they won't ever see the extra install's happening. Or we could update shims.js to specify proper version instead of commit tag, so the semver check in cmd.js would work properly, that's another possible solution.

I'm ok to try implementing the YAML parsing and version check, if you think that's the right way to go though.

[mvayngrib]

u have an interesting point about integrity checks. However, to be consistent with the behavior for the other shims, I would say let's avoid the extra install if possible.

i think this module has the official parser: https://www.npmjs.com/package/@yarnpkg/lockfile

[jd20]

Ok, let me try it out. I'll see if the environment variable is still necessary or not, too.

[jd20]

Actually, I noticed this problem is not yarn-specific, when I run npm i repeatedly, both react-native-http and readable-stream keep getting re-installed (for same reason, the semver check between a commit tag and the version from package.json will fail). Maybe we should only compare the semver in shims.js against the resolved version in package-lock.json / yarn.lock, instead of node_modules/*/package.json. That should fix the extra installs for both npm and yarn.

[mvayngrib]

@jd20 the rabbit hole gets deeper :) In npm@3 there is no package-lock.json. So maybe check yarn.lock/package-lock.json if they exist, otherwise fall back to the current check (which works for npm@3)

[mvayngrib]

maybe we're taking it too far. Maybe we just need to be clear that the --install flag does not belong in the "postinstall" script, and should only be run once in a while when you need new shims.

[jd20]

@mvayngrib Too late, I think I'm almost done :) Fallback to old logic if package-lock.json doesn't exist makes sense.

[mvayngrib]

haha cool, good luck in the home stretch!

[jd20]

I think this is working correctly now, I was able to get rid of the environment var flag as well. I'm only seeing two issues when testing with npm: 1) readable-stream keeps getting re-installed, because npm is not installing the version requested (yarn doesn't exhibit this problem, and if I install readable-stream with yarn first, then the issue goes away with npm, so probably just some npm non-determinism). 2) The logic for comparing long / short commit tags in the version is kind of basic, so you could fool it if you had two branches (like "#fix" and "#fix2", it would think these are equivalent). This isn't really new though, the fallback logic can be fooled the same way, so I'm not worrying about it too much.

And yes, removing --install from the postinstall command makes everything a lot simpler and work more reliably :)

[mvayngrib]

@jd20 awesome, i'll try to review by end of tomorrow

[mvayngrib]

@jd20 eslint is telling me there's an undefined var (see below). I added some basic "pre-commit" linting in the master branch. Could you rebase and fix? Thanks! npm install after rebase to hook up husky for pre-commit linting. We don't have tests yet, so let's at least lint :)

  ...
  171:33  error  'pkgJson' is not defined                       no-undef
  ...

[jd20]

Good catch! I pulled from upstream, fixed the eslint warnings, and re-tested, with package-lock=false this time so the lockfile wouldn't get recreated (standard npm v5 behavior). Local testing looked good, please take a look again.

[mvayngrib]

@jd20 looks good, thanks! Will merge and release a minor version

[jd20]

Sounds great! 🎉