search

Galooshi / happo

Visual diffing in CI for user interfaces
504 stars 16 forks source link

WIP: React Native Windows Support #193

Open ryanlntn opened 7 years ago

ryanlntn commented 7 years ago

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.

lelandrichardson commented 7 years ago

cc @spikebrehm as well

lelandrichardson commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

@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.