Add support for React Native #179

lelandrichardson · 2017-01-06T02:06:36Z

This PR is a work in progress, and so far only works for ios (but is almost ready for android). It's best viewed by commit, as it includes a lot of RN template code in the first 2 commits.

Ultimately, I think it makes sense for us to implement this as a plugin system or something, so this PR will likely never get merged, but I thought it would be a good way for us to start the discussion.

I haven't made much of an attempt to share code, though I did refactor some methods into modules so they could be shared across implementations.

How it works:

running happo run-ios does the following things:

Starts a websockets server that the ios app will use to communicate between the two processes
Starts appium, which then launches the simulator with the happo app
The happo app starts, and requests an RN bundle from localhost:8081/happo.bundle which is assuming that the consumer of happo has the RN packager turned on (to default port of 8081) with a happo.js entry file at the root.
The happo.js file needs to at some point require the happo/native js module, which will register itself with the happo server, and the happo app.
Consumer of happo will register happo stories with import StoryManager from 'happo/native';
At startup, happo will send the list of story names over websockets to happo, then render them one by one, taking a snapshot of each, and saving to a temp file. Temp files + story names will be sent across websockets to the happo server.
Happo server diffs files appropriately and does its thing

lencioni

This seems like a great start!

lencioni · 2017-01-07T16:55:11Z

native/js/HappoJob.js

+  render() {
+    const { story } = this.state;
+    return (
+      <View style={{ paddingTop: 20 }}>


Why padding on the top?

i didn't check if they did or not, but i was worried that the screenshots would include the status bar

lencioni · 2017-01-07T16:57:15Z

native/js/Scaffold.js

+import React, { Component } from 'react';
+import HappoJob from './HappoJob';
+
+class Scaffold extends Component {


Does RN not support SFCs?

Stateless function components? Looks like it should work: https://medium.com/front-end-hacking/stateless-components-in-react-native-e9034f2e3701#.z3v1kwmvf

they work. i avoid them mostly just because they break hot module reloading

lencioni · 2017-01-07T16:59:09Z

native/js/StoryRenderer.js

+  render() {
+    const { Component } = this.props;
+    return (
+      <View ref={this.setRef} onLayout={this.onLayout} style={styles.container}>


Does this shrink to be the size of what it contains or does it work more like display: block?

lencioni · 2017-01-07T16:59:39Z

.gitignore

+
+# node.js
+#
+node_modules/


We already have this

lol this list is long 🚋

lencioni · 2017-01-07T17:00:50Z

package.json

@@ -55,7 +58,12 @@
    "mkdirp": "^0.5.1",
    "png-crop": "0.0.1",
    "pngjs": "^2.3.1",
+    "react": "15.4.1",


Should we remove this from devDepedencies at the same time? I suppose it won't matter once we actually do the plugin stuff we talked about.

lencioni · 2017-01-07T17:08:07Z

src/server/runIOSVisualDiffs.js

+      (snapshots) => {
+        resolve(snapshots);
+      }
+    ).then(initializeIOSdriver);


We have the initializeWebdriver call in the cli script, but you put initializeIOSdriver inside of runIOSVisualDiffs. I don't think it matters too much where it goes either way, but I think it would be good to have consistency.

I think I did that to make it easier to test runVisualDiffs.

lencioni · 2017-01-07T17:09:40Z

src/server/runIOSVisualDiffs.js

+  return new Promise((resolve) => {
+    setupWebsockets(
+      (/* name, uri */) => {
+        // do nothing


What is this callback for? This comment would be clearer if it explained when we are doing nothing and why we are doing nothing.

Alternatively, maybe setupWebsockets should take an object argument instead of positional arguments? That would make this clearer.

lencioni · 2017-01-07T17:11:51Z

src/server/runIOSVisualDiffs.js

+}
+
+module.exports = function runIOSVisualDiffs() {
+  const viewportName = 'ios';


I'm not sure this is quite right. There different sizes on ios, right (e.g. big iphone, small iphone, ipad)? If so, then we'd want this to match at least one of those sizes. Ideally, this would be configurable so you could take screenshots in as many or few of the sizes as you like. Unless maybe that doesn't ever come into play in an ios app? I honestly have no idea how any of that works.

lencioni · 2017-01-07T17:14:10Z

src/server/saveResultToFile.js

@@ -0,0 +1,18 @@
+function saveResultToFile(runResult) {


This needs to be exported, eh?

lencioni · 2017-01-07T17:16:15Z

src/server/setupWebsockets.js

+
+  return new Promise(resolve => {
+    http.listen(5000, function() {
+      console.log('listening on *:5000');


Should this comment mention what is listening on this port?

trotzig · 2017-01-07T18:20:52Z

native/ios/happo.xcodeproj/xcshareddata/xcschemes/happo.xcscheme

+      buildConfiguration = "Release"
+      revealArchiveInOrganizer = "YES">
+   </ArchiveAction>
+</Scheme>


I'm not familiar with iOS development, but I'm assuming that this is generated in some way. Are there any parts of this configuration that "stand out" that are worth mentioning? Or is it mostly a standard setup?

trotzig · 2017-01-07T18:21:48Z

native/ios/happo/AppDelegate.h

+ * This source code is licensed under the BSD-style license found in the
+ * LICENSE file in the root directory of this source tree. An additional grant
+ * of patent rights can be found in the PATENTS file in the same directory.
+ */


Do these need to be here?

trotzig · 2017-01-07T18:27:11Z

native/android/app/BUCK

+#
+
+lib_deps = []
+for jarfile in glob(['libs/*.jar']):


Do people normally add dependencies to source control or manage them through maven/ivy etc for android apps? This isn't important right now, I'm mostly asking to make sure I'm understanding what's going on.

trotzig · 2017-01-07T18:31:42Z

native/android/gradlew

+eval splitJvmOpts $DEFAULT_JVM_OPTS $JAVA_OPTS $GRADLE_OPTS
+JVM_OPTS[${#JVM_OPTS[*]}]="-Dorg.gradle.appname=$APP_BASE_NAME"
+
+exec "$JAVACMD" "${JVM_OPTS[@]}" -classpath "$CLASSPATH" org.gradle.wrapper.GradleWrapperMain "$@"


Is this copied/generated from somewhere or did you hand-write it? I'm assuming copied/generated.

trotzig · 2017-01-07T18:32:56Z

native/index.js

+// this is needed to support socket.io
+window.navigator.userAgent = 'react-native';
+
+const { AppRegistry } = require('react-native');


Could we move this to the top? Or is the import depending on userAgent to be set?

trotzig · 2017-01-07T18:34:26Z

native/js/HappoJob.js

+} from 'react-native';
+import io from 'socket.io-client';
+import StoryRenderer from './StoryRenderer';
+import { getStories, getStory } from './StoryManager';


I like the name "story". Assuming it's the same thing that we use for "example" in the js version, I could consider changing it globally to "story".

trotzig · 2017-01-07T18:35:53Z

native/js/HappoJob.js

+
+HappoJob.propTypes = propTypes;
+
+module.exports = HappoJob;


Looking at this makes me think that we should switch to using React for rendering examples/stories in the browser version as well.

trotzig · 2017-01-07T18:36:58Z

native/js/Scaffold.js

+import React, { Component } from 'react';
+import HappoJob from './HappoJob';
+
+class Scaffold extends Component {


Stateless function components? Looks like it should work: https://medium.com/front-end-hacking/stateless-components-in-react-native-e9034f2e3701#.z3v1kwmvf

trotzig · 2017-01-07T18:40:17Z

.gitignore

+
+# node.js
+#
+node_modules/


lol this list is long 🚋

trotzig · 2017-01-07T18:41:23Z

package.json

@@ -8,6 +8,8 @@
  "scripts": {
    "build": "npm run --silent clean && babel src/server/ -d lib/server/ --ignore __tests__ && npm run --silent webpack",
    "clean": "rimraf lib public/*.worker.js public/*.bundle.js*",
+    "babel:watch": "babel src/server -d lib/server --ignore __tests__ --watch",


I've been meaning to add this for a while, thanks! I do think we could extract a separate babel step so that we don't have to duplicate the params in build.

trotzig · 2017-01-07T18:47:16Z

src/server/runIOSVisualDiffs.js

+      (snapshots) => {
+        resolve(snapshots);
+      }
+    ).then(initializeIOSdriver);


I think I did that to make it easier to test runVisualDiffs.

trotzig · 2017-01-07T18:54:48Z

Thanks for working on this! I'm excited to see the progress.

As someone who hasn't done any iOS or Android development, what are the steps necessary to try this out? You don't have to be specific, just list e.g "install xcode", "pay for developer's license", "read up on objective-c" etc. :)

@lelandrichardson

This work was prepared by @lelandrichardson as part of PR #179. I'm cherry-picking them here to simplify work going forward. These methods are all general-purpose, and should be possible to reuse over multiple targets.

lelandrichardson · 2017-01-17T19:35:23Z

Closing in favor of #185

lelandrichardson added 4 commits January 5, 2017 17:35

Add RN iOS Project

353f27c

Add RN Android Project

5f24af2

Add RN JS

1d88d5f

Add Happo CLI RN Process

95245ff

lencioni reviewed Jan 7, 2017

View reviewed changes

trotzig reviewed Jan 7, 2017

View reviewed changes

lelandrichardson closed this Jan 17, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for React Native #179

Add support for React Native #179

lelandrichardson commented Jan 6, 2017

lencioni left a comment

lencioni Jan 7, 2017

lelandrichardson Jan 8, 2017

lencioni Jan 7, 2017

trotzig Jan 7, 2017

lelandrichardson Jan 8, 2017

lencioni Jan 7, 2017

lencioni Jan 7, 2017

trotzig Jan 7, 2017

lencioni Jan 7, 2017

lencioni Jan 7, 2017

trotzig Jan 7, 2017

lencioni Jan 7, 2017

lencioni Jan 7, 2017

lencioni Jan 7, 2017

lencioni Jan 7, 2017

lencioni Jan 7, 2017

trotzig Jan 7, 2017

trotzig Jan 7, 2017

trotzig Jan 7, 2017

trotzig Jan 7, 2017

trotzig Jan 7, 2017

trotzig Jan 7, 2017

trotzig Jan 7, 2017

trotzig Jan 7, 2017

trotzig Jan 7, 2017

trotzig Jan 7, 2017

trotzig Jan 7, 2017

trotzig commented Jan 7, 2017

lelandrichardson commented Jan 17, 2017


		HappoJob.propTypes = propTypes;

		module.exports = HappoJob;

Add support for React Native #179

Add support for React Native #179

Conversation

lelandrichardson commented Jan 6, 2017

lencioni left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trotzig commented Jan 7, 2017

lelandrichardson commented Jan 17, 2017