Add React/Jest testing infrastructure #1118

trueadm · 2017-10-31T11:41:36Z

Release note: none

This PR adds the React/Jest testing infrastructure to Prepack and is stacked on the PR #1117. Thus, that PR should be merged before this PR is merged to reduce noise in the diff.

This will allow us to test various runtime outputs of React 16 when running original source vs Prepacked source to see if there are any issues/differences that might have an impact on applications. The primary reason for this is to track regressions for the component folding PRs that will land in the future.

Please note, this PR does not contain any reconciler changes, thus __registerReactComponentRoot(App); has been commented out of tests to ensure they don't prematurely fail. A follow up PR will enable them once the other React functional component folding PRs get landed.

This PR also adds some mock React globals to be used within tests (and maybe to be further integrated into folding at a future point too). The mocks include React.Component and React.cloneElement for now.

Furthermore, a utils/json.js utility file exists to help normalize the results from the React test renderer so that adjacent JSON text nodes get merged, which is something that may exist because of how the reconciler (once the PR lands) handles inlining of child nodes.

The command to run React tests is yarn test-react.

gaearon

Looks OK to me aside from style nits and some things I don't fully understand.
I reviewed bcb80bd.

gaearon · 2017-10-31T13:58:03Z

scripts/test-react.js

+let React = require("react");
+let ReactTestRenderer = require("react-test-renderer");
+let { normalize } = require("../lib/utils/json.js");
+/* eslint-disable no-undef */


Doesn't this disable them globally for the whole file? I think you want // eslint-disable-next-line or something like that.

gaearon · 2017-10-31T13:59:51Z

scripts/test-react.js

+  let code = `(function(){${source}})()`;
+  let serialized = prepackSources([{ filePath: "", fileContents: code, sourceMapContents: "" }], prepackOptions);
+  // add the React require back in, as we've removed it with our Prepack mock
+  let compiledSource = serialized.code.replace(/_\$[\d].React/, "React = require('react')");


Let's add a comment explaining what the regex is trying to find.

gaearon · 2017-10-31T14:01:56Z

scripts/test-react.js

+  let resultB = getTrials(rendererB, B);
+
+  // // the test has returned many values for us to check
+  if (Array.isArray(resultA) && Array.isArray(resultA[0])) {


Can we restructure this so that instead of repeating similar logic in both branches, we first wrap resultA and resultB into an array if necessary and then do the assertions unconditionally?

gaearon · 2017-10-31T14:02:55Z

scripts/test-react.js

+      // with a false-negative error
+      global.console.error = () => {};
+      await runTest(directory, "return-undefined.js");
+      global.console.error = originalConsoleError;


Maybe try { } finally { } so this is isolated from other tests in case it fails?

I thought this wasn't actual JS error that is occurring, rather it's console.error being fired?

If runTest fails for some reason, global.console.error won't be restored.

If you do

global.console.error = ... try { doStuff } finally { global.console.error = original }

it is restored no matter what.

gaearon · 2017-10-31T14:03:19Z

src/intrinsics/react/global.js

+  let requireValue = AbstractValue.createFromTemplate(
+    realm,
+    buildExpressionTemplate("require"),
+    (type: any), // Flow complains that type of Value isn't compatible with class Value :/


Would be nice to understand why.

gaearon · 2017-10-31T14:06:40Z

src/utils/json.js

+type JSON = { [key: string]: JSONValue };
+
+// this will mutate the original JSON object
+export function normalize(node: JSON) {


This utility name sounds very generic but it's not very clear what this does.

What is normalized JSON looking like?
Is it useful in any other places than this test infra?

gaearon · 2017-10-31T14:07:10Z

test/react/functional-components/clone-element.js

+if (this.__createReactMock) {
+  var React = __createReactMock();
+} else {
+	var React = require('react');


Nit: indent

gaearon · 2017-10-31T14:07:26Z

test/react/functional-components/conditional.js

+
+function App() {
+	return (
+		<MaybeShow show={true}>


Nit: indent

(and in most places below)

gaearon · 2017-10-31T14:08:21Z

test/react/functional-components/dynamic-context.js

+    superclass.apply(this, arguments);
+  }
+
+  if ( superclass ) StatefulComponent.__proto__ = superclass;


Nit: style is odd, can we use the same code style as in code?

gaearon · 2017-10-31T14:09:03Z

test/react/functional-components/dynamic-context.js

+  return StatefulComponent;
+}(React.Component));
+
+this.StatefulComponent = StatefulComponent;


Why do we need this.* assignment here but not in other places?

sebmarkbage · 2017-10-31T16:54:29Z

The move from test/serializer/react/→ test/serializer/jsx/. That implementation of JSX is still React specific. I've tried to keep the brand name around JSX separate from React. So it would be good if we could call this something with React in it. test/serializer/react/jsx/ maybe?

sebmarkbage · 2017-10-31T18:19:09Z

src/intrinsics/prepack/global.js

@@ -155,6 +155,39 @@ export default function(realm: Realm): void {
    configurable: true,
  });

+  if (realm.react.enabled) {
+    global.$DefineOwnProperty("__reactComponentRoots", {


@cblappert This is unrelated to this PR. I saw that this pattern was added in the additional functions one, but why do we make these a public object instead of just an internal Map? Seems weird for the compiled script to be able to reason about this and mutate this.

facebook-github-bot

@trueadm has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot

@trueadm has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Summary: Release note: Adds experimental React functional component folding optimizations This PR is stacked upon PRs #1118 and #1117. Thus, those PRs should be merged before this PR is merged to reduce noise in the diff. This PR adds a new React Reconciler into Prepack's serialization process, so that React components trees can be folded/inlined into a single component at build time. To fold a component tree, it must be explicitly done via `__registerReactComponentRoot(nameOfComponent)`. This PR only attempts to fold React functional components, not React ES2015 class components (that will come in another PR at a later date). Furthermore, the `props` parameter on a root component must contain Flow type annotations (otherwise we will have no idea what the values might be). Support flow `propTypes` might also be an addition, but not for this PR. If the reconciler comes across a component that it cannot fold/inline, it will "bail-out" and try and continue the process without that particular component being folded into the tree. An example of how this all works (input): ```jsx function App(props: {title: string}) { return ( <div> <ChildComponent title={props.title} /> </div> ); } function ChildComponent(props) { return ( <span> <SubChildComponent {...props} /> </span> ); } function SubChildComponent(props) { return <span>{props.title.toString()}</span> } __registerReactComponentRoot(App); global.App = App; ``` Output: ```jsx (function () { "use strict"; var _$1 = this; var _0 = function (props) { var _$0 = props.title; return <div><span><span>{_$0}</span></span></div>; }; _$1.App = _0; }).call(this); ``` Closes #1120 Differential Revision: D6237333 Pulled By: trueadm fbshipit-source-id: b58c7d8979ca79a766bb2ee2eb01a380d37c3101

trueadm added 3 commits October 31, 2017 11:04

A small cleanup and preperation work for React component folding

3701984

added invariant check back in to checkReactRootComponents

0da8119

Adds testing infra for React

bcb80bd

trueadm requested review from sebmarkbage and gaearon October 31, 2017 11:41

facebook-github-bot added the CLA Signed label Oct 31, 2017

Comment out flow types from tests

a593124

gaearon approved these changes Oct 31, 2017

View reviewed changes

Addresses feedback in PR comments

24839c3

trueadm requested review from NTillmann and cblappert October 31, 2017 14:31

trueadm mentioned this pull request Oct 31, 2017

Add React functional component folding #1120

Closed

moved files as per comment

d963898

cblappert approved these changes Oct 31, 2017

View reviewed changes

sebmarkbage reviewed Oct 31, 2017

View reviewed changes

sebmarkbage approved these changes Oct 31, 2017

View reviewed changes

trueadm added 3 commits November 1, 2017 11:31

fix conflicts

2aca893

add try/catch to restore console error

acb2999

add try/catch/finally to restore console error

084fcf0

facebook-github-bot reviewed Nov 1, 2017

View reviewed changes

Updated yarn.lock

a692607

facebook-github-bot reviewed Nov 1, 2017

View reviewed changes

facebook-github-bot closed this in e63daa5 Nov 2, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add React/Jest testing infrastructure #1118

Add React/Jest testing infrastructure #1118

trueadm commented Oct 31, 2017 •

edited

Loading

gaearon left a comment •

edited

Loading

gaearon Oct 31, 2017

gaearon Oct 31, 2017

gaearon Oct 31, 2017

gaearon Oct 31, 2017

trueadm Oct 31, 2017

gaearon Oct 31, 2017

gaearon Oct 31, 2017

gaearon Oct 31, 2017

gaearon Oct 31, 2017

gaearon Oct 31, 2017

gaearon Oct 31, 2017

gaearon Oct 31, 2017

gaearon Oct 31, 2017

sebmarkbage commented Oct 31, 2017

sebmarkbage Oct 31, 2017

facebook-github-bot left a comment

facebook-github-bot left a comment

Add React/Jest testing infrastructure #1118

Add React/Jest testing infrastructure #1118

Conversation

trueadm commented Oct 31, 2017 • edited Loading

gaearon left a comment • edited Loading

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

sebmarkbage commented Oct 31, 2017

Choose a reason for hiding this comment

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot left a comment

Choose a reason for hiding this comment

trueadm commented Oct 31, 2017 •

edited

Loading

gaearon left a comment •

edited

Loading