WIP: React Native Windows Support

[ryanlntn]

Hi! I'm working on adding React Native Windows support to the React Native target. So far I've ported react-native-view-shot to Windows and updated the target code the best I can to support the Windows platform but I'm running into a snag when I actually try to run it against the example project. It seems as though the example project isn't being run inside the host RN app but outside where it fails with __DEV__ is undefined. I'm still trying to work this out on my end but thought I'd open this PR in case there was something obvious I was missing that you could point out.

[trotzig]

I'm excited to see this happen! I haven't done much RN at all, so I can't really help you in any good way right now. I know @lelandrichardson (who added RN support for iOS and Android) has been rather busy lately, but hopefully he could chime in here.

[trotzig]

good catch!

[lelandrichardson]

cc @spikebrehm as well

[lelandrichardson]

just curious, but does having react-native and react-native-windows both in our dev deps preclude us from doing non react-native-windows development? I'm somewhat ignorant of how this works.

[ryanlntn]

@lelandrichardson It doesn't preclude non react-native-windows development but it is a fairly large dependency to include if you're not working on windows. I was thinking of including as an optional dependency but I wasn't sure if that was the right approach either.

[spikebrehm]

Interesting. I'm tempted to agree this should be an optionalDependency. We could even create happo-target-react-native-windows, which would include this dependency. It may be a bit more work to modularize the code properly then.

[lelandrichardson]

optionalDependency would be worse, as that would make all of our users install it as well

[lelandrichardson]

Other than the included sample project, this looks like a very simple addition - so that's good!

One thing to keep in mind is that @spikebrehm and I were discussing not including the runner app as part of the target... but it's really not clear what the right thing to do is at this point. Internally, we've found the runner project to not be all that useful because in order to have meaningful screenshots we need to add in a lot of our own stuff (like fonts for instance), so we end up just creating a small runner project ourselves and don't use the one inside the npm package.

I'm curious if @spikebrehm has any thoughts here.

[spikebrehm]

LGTM! I'm not able to test it without a Windows machine, but I trust you've tested it and it works.

@ryanlntn are you available to provide code reviews for any PRs that touch the Windows stuff?

[spikebrehm]

The build is failing due to ESLint errors:

/home/travis/build/Galooshi/happo/packages/happo-target-react-native/src/defaultOptions.js
  31:2   error  Missing semicolon       semi
  70:28  error  Missing trailing comma  comma-dangle
  71:4   error  Missing trailing comma  comma-dangle
/home/travis/build/Galooshi/happo/packages/happo-target-react-native/src/native/StoryManager.js
  19:26  error  Trailing spaces not allowed  no-trailing-spaces

[spikebrehm]

One thing to keep in mind is that @spikebrehm and I were discussing not including the runner app as part of the target... but it's really not clear what the right thing to do is at this point. Internally, we've found the runner project to not be all that useful because in order to have meaningful screenshots we need to add in a lot of our own stuff (like fonts for instance), so we end up just creating a small runner project ourselves and don't use the one inside the npm package.

Yeah, the iOS/Android HappoRunner app is dead code at the moment. Like @lelandrichardson said, we end up having to include things specific to our app, such as fonts and certain native modules. Our situation is pretty far in the "brownfield" direction; I wonder if for a typical "greenfield" RN app, written almost entirely in JS, if the HappoRunner app would be applicable. To determine this, we could try adding Happo to one of the open-source RN apps, like https://github.com/Thinkmill/react-conf-app. I'm also curious about adding the Happo entry point directly to the main app itself, rather than requiring a separate app for it; however, this added bloat added to a production app may not be acceptable for most people.

If we find that most apps require some customization of the HappoRunner app, then perhaps we should pull HappoRunner out to a separate module and position it as an example app to be copied.

In any case, this isn't a blocker for this PR!

[ryanlntn]

@spikebrehm I'm actually still having trouble connecting everything. At this point I have the packager and driver initializing and resolving but the runner app is failing with a Cannot resolve __fbBatchedBridge before any screenshots are taken. Considering the other runners are dead code I'm curious what setting up a runner should look like and if I'm missing something from my runner.