Fiber test renderer #8628

Merged
merged 33 commits into from Jan 11, 2017

Projects

None yet

6 participants

@iamdustan
Contributor
iamdustan commented Dec 22, 2016 edited

This is the start of reimplementing ReactTestRenderer on Fiber. Current tests status:

  • renders a simple component (2ms)
  • renders a top-level empty component
  • exposes a type flag (2ms)
  • can render a composite component (1ms)
  • renders some basics with an update
  • exposes the instance (1ms)
  • updates types (1ms)
  • updates children
  • does the full lifecycle (1ms)
  • gives a ref to native components (2ms)
  • warns correctly for refs on SFCs
  • allows an optional createNodeMock function
  • supports unmounting when using refs
  • supports unmounting inner instances (3ms)
  • supports updates when using refs
  • supports error boundaries
  • compatibility: add support for unstable_batchedUpdates

A good chunk of those tests have to do with refs and the ability to provide a createNodeMock function. I believe that ReactFiberReconciler will need to be updated with a getPublicInstance method to handle this.

Ignore all changes in ReactTestRenderer-test.js. The first is to import the fiber renderer instead of stack (actually, is there a better way to do this? should I copy the ReactTestRenderer-test for ReactTestFiberRenderer?), the rest are testing/skip/etc and will be backed out when done.

cc @spicyj

@aweary
Collaborator
aweary commented Dec 22, 2016 edited

should I copy the ReactTestRenderer-test for ReactTestFiberRenderer?

👍 I think we'll want to keep the tests for the stack implementation of ReactTestRenderer for now

+
+var instanceCounter = 0;
+
+var ReactTestFiberComponent = {
@gaearon
gaearon Dec 22, 2016 Member

Note: the separation between ReactDOMFiber and ReactDOMFiberComponent is just an artifact of how it was forked from ReactDOMComponent and that we still want to keep them in sync when possible. So feel free to abandon that convention (e.g. see ReactNoop.js)

@spicyj
Member
spicyj commented Dec 22, 2016

should I copy the ReactTestRenderer-test for ReactTestFiberRenderer?

It would be fine to branch on renderer in the test file like the ReactART tests do.

@iamdustan
Contributor

Thanks for the feedback thus far.

Regarding refs I think I can reuse the Renderer.findHostInstance that already exists, though I don’t think I understand the value it is returning. For example:

    class Component extends React.Component {
      render() {
        return (
          <div className="purple">
            <div />
            <Child />
          </div>
        );
      }
    }

    class Child extends React.Component {
      render() {
        return (
          <div className="green" />
        );
      }
    }
    
    ReactTestRenderer.create(<Component />);
    const hostInstance = TestRenderer.findHostInstance(root);
    /* hostInstance ==>
{ type: 'div',
      children:
       [ { '$$typeof': Symbol(react.element),
           type: 'div',
           key: null,
           ref: null,
           props: {},
           _owner: [Object],
           _store: {} },
         { '$$typeof': Symbol(react.element),
           type: [Function: Child],
           key: null,
           ref: null,
           props: {},
           _owner: [Object],
           _store: {} },
         { type: 'div', children: null, props: {} },
         { type: 'div', children: null, props: [Object] } ],
      props: { className: 'purple' } }
*/

Is that expected? Why?

@iamdustan
Contributor

Second issue is if I try to call findHostInstance on the (currently passed) Fiber to a ref callback I always get it as in an unmounted state.

    class Component extends React.Component {
      render() {
        return (
          <div className="purple">
            <div />
            {/* this callback ref will throw */}
            <Child ref={fiber => console.log(ReactTestRenderer.findHostInstance(fiber)} />
          </div>
        );
      }
    }

    class Child extends React.Component {
      render() {
        return (
          <div className="green" />
        );
      }
    }
    ReactTestRenderer.create(<Component />);

// ref calback throws:
// Invariant Violation: Unable to find node on an unmounted component.

I would expect that to be mounted by the time the ref callback is called...

@iamdustan
Contributor

Actually, ignore everything I’ve said. I think I see a path forward.

@iamdustan
Contributor

Woohoo! Only 2 failing tests now!

I have ReactTestRenderer-test branching on the ReactDOMFeatureFlags useFiber switch now.

To support the createNodeMock function I needed to add a method to ReactFiberHostContext to support support hijacking the attachRef call in ReactFiberCommitWork. I’m a bit iffy about adding that since it seems intentional in the renderer API that that instances a renderer creates are exactly what the user gets in a ref.

With the two failing tests, I am curious if these are failing because the feature is incomplete or because behavior has changed subtly.

warns correctly for refs on SFCs

Is this warning still to be implemented? If yes I would suspect it to “just work” here without any additional effort.

supports updates when using refs

The ref callback is not being called yet on unmount. This is another situation where I would expect this to just work. Is this not yet implemented or do I need to track down a bug in this renderer?

supports error boundaries

The order of ilfecycles is different here. Based on watching the error boundary work between @acdlite and @gaearon I suspect that this may be expected.

Before

        'Boundary render',
        'Angry render',
        'Boundary render',
        'Boundary componentDidMount',

After

        'Boundary render',
        'Angry render',
        'Boundary componentDidMount',
        'Boundary render',
@iamdustan
Contributor

Just checked master. It looks like warns correctly for refs on SFCs is not implemented yet. That answers one of my questions.

@gaearon
Member
gaearon commented Dec 24, 2016

@iamdustan Want to help with implementing that warning in master as a separate PR?

@@ -86,7 +86,11 @@ module.exports = function<T, P, I, TI, C, CX>(
function attachRef(current : ?Fiber, finishedWork : Fiber, instance : any) {
const ref = finishedWork.ref;
if (ref && (!current || current.ref !== ref)) {
- ref(instance);
+ if (typeof config.getPublicInstance === 'function') {
@gaearon
gaearon Dec 24, 2016 Member

Please destructure this from config earlier. We don't want to read that value every time. I think we could also make this required to keep contract strict.

+ parentHostContext : HostContext,
+ type : string,
+ ) : HostContext {
+ return {};
@gaearon
gaearon Dec 24, 2016 Member

Please return the same object (emptyObject like in ReactNoop) so it doesn't push stack unnecessarily.

@sebmarkbage sebmarkbage referenced this pull request Jan 3, 2017
Open

[Fiber] Umbrella for remaining features / bugs #7925

67 of 98 tasks complete
@gaearon
Member
gaearon commented Jan 9, 2017

Can you please update this now that the SFC warning is in?

@iamdustan
Contributor

The only failing test remaining is that it supports updates when using refs. I just copied that test to refs-test locally to see if the behavior already works in ReactDOMFiber with a slight change to account for null being passed on unmount (rather than the same node).

The test expects the ref to be called with ['div', 'div', 'span'] (the second div being on unmount), whilst the current behavior is to only be called with ['div', 'span'].

My biggest concern is that being called twice wasn’t necessarily an intentional decision before, but the test was added to assert the behavior and not regress accidentally. I’m not certain how much effort should be put into maintaining this because I think it would require changing ReactFiberCommitWorks detachRefIfNeeded to call the createNodeMock method in some capacity.

@spicyj what thoughts or advise do you have since you wrote the original implementation?

+ createNodeMock: Function;
+
+ constructor(rootID, createNodeMock) {
+ this.rootID = rootID;
@gaearon
gaearon Jan 10, 2017 Member

I don't see rootID being used anywhere. Is it needed?

+ }
+
+ update(type, props) {
+ // console.log('update', type, props, this.children);
@gaearon
gaearon Jan 10, 2017 Member

Comments

+ }
+
+ toJSON() {
+ // eslint-disable ignore the children
@gaearon
gaearon Jan 10, 2017 Member

What are we ignoring specifically? Generally I'd like to avoid adding eslint ignores.

+ if (typeof child.toJSON === 'function') {
+ childrenJSON.push(child.toJSON());
+ } else if (typeof child.text !== 'undefined') {
+ childrenJSON.push();
@gaearon
gaearon Jan 10, 2017 Member

Are pushing undefined? Why?

+ rootContainerInstance : Container,
+ ) : boolean {
+ // console.log('finalizeInitialChildren');
+ // setInitialProperties(testElement, type, props, rootContainerInstance);
@gaearon
gaearon Jan 10, 2017 Member

Please clean up the comments.

+ );
+ },
+
+ resetTextContent(testElement : Instance) : void {},
@gaearon
gaearon Jan 10, 2017 Member

Nit: Could you please use a consistent style for empty methods? Either like this, or with // Noop comment as long as it's the same style everywhere.

+ text : text,
+ id: instanceCounter++,
+ rootContainerInstance,
+ toJSON: () => isNaN(+inst.text) ? inst.text : +inst.text,
@gaearon
gaearon Jan 10, 2017 Member

I'm not sure this is right. For example even "2" would become 2. Maybe this is unobservable implementation detail and we should just always use strings? Since that's what they get coerced to anyway.

@iamdustan
iamdustan Jan 10, 2017 Contributor

good point. this was to make one of the tests pass, but I think to be correct I need to capture the type when creating the TextInstance and only coerce back to a number if it came in as a number.

@gaearon
gaearon Jan 10, 2017 Member

I don't think you'd actually get a number though in createTextInstance? As far as I can see it's already coerced by Fiber code by then. I think it actually makes more sense than exposing this to the renderer, and I'm fine with divergent behavior here (always treating text as strings).

+ parentInstance.removeChild(child);
+ },
+
+ scheduleAnimationCallback: window.requestAnimationFrame,
@gaearon
gaearon Jan 10, 2017 Member

I think this will throw in non-jsdom environment. Can you make it setTimeout? It seems like it wouldn't be used anyway, but at least we shouldn't throw.

+ }
+ var container = new TestContainer('<default>', createNodeMock);
+ var root = TestRenderer.createContainer(container);
+ if (root) {
@gaearon
gaearon Jan 10, 2017 Member

When is it null?

@iamdustan
iamdustan Jan 10, 2017 Contributor

I don’t think ever, but flow was complaining that TestRenderer.createContainer may return null

@gaearon
gaearon Jan 10, 2017 Member

You'd need to see why other renderers didn't need this. It probably inferred that OpaqueNode type can be null, maybe from some other method definition.

+ },
+
+ /* eslint-disable camelcase */
+ unstable_batchedUpdates() {
@gaearon
gaearon Jan 10, 2017 Member

Can you just omit it? I don't think we want to expose it here.

@iamdustan
iamdustan Jan 10, 2017 Contributor

Oh yeah, forgot about this one. The stack-based ReactTestRenderer exposes this currently..

@gaearon
gaearon Jan 10, 2017 Member

Exposing but throwing isn't much better. :-)
Let's either make it work or remove it.

@iamdustan
iamdustan Jan 10, 2017 Contributor

😇

...adding this to the PR checklist.

@@ -47,6 +47,7 @@ export type HostConfig<T, P, I, TI, C, CX, CI> = {
getRootHostContext(rootContainerInstance : C) : CX,
getChildHostContext(parentHostContext : CX, type : T) : CX,
+ getPublicInstance(instance : I | TI) : any, // maybe add a PI (public instance type)?
@gaearon
gaearon Jan 10, 2017 Member

Please do.

@@ -317,6 +343,7 @@ describe('ReactTestRenderer', () => {
]);
});
+ // this is only passing in Fiber because refs aren't actually working
@gaearon
gaearon Jan 10, 2017 Member

Is this comment outdated? It’s not telling me much: are refs broken in Fiber? Are they broken specifically for test renderer? What is our plan to fix them? Is this within the scope of this PR?

@iamdustan
iamdustan Jan 10, 2017 Contributor

outdated comment.

+ }
+ ReactDOM.render(<Foo useDiv={true} />, container);
+ ReactDOM.render(<Foo useDiv={false} />, container);
+ expect(log).toEqual(['div', null, 'span']);
@gaearon
gaearon Jan 10, 2017 Member

Ideally also unmount at the end and check we get another null?

@iamdustan
iamdustan Jan 10, 2017 Contributor

oh whoops, didn’t mean to actually commit this. This was testing the behavior difference for refs between the test-renderer and DOM renderer re #8628 (comment)

@gaearon
Member
gaearon commented Jan 10, 2017 edited

I think the existing test for updates might be checking accidental implementation details.

It's not testing refs themselves, it's testing calls of createNodeMock.
It was added by @aweary in e43aaab, not by @spicyj.

In particular, I don't see why it calls createNodeMock for div twice. It might just be a bug/inefficiency in the Stack version. Why would you need to mock a node for something you're unmounting?

@aweary
Collaborator
aweary commented Jan 10, 2017 edited

Right now createNodeMock is always called when getPublicInstance is called, and when unmounting ReactOwner.removeComponentAsRefFrom calls it to see if the cached ref is equal to the current public instance.

We can try caching the mock node on the instance and only calling createNodeMock when the instance is being mounted/updated.

@iamdustan
Contributor
iamdustan commented Jan 10, 2017 edited

thanks @gaearon and @aweary. I added the caching and changed the test case accordingly in 087f1f6. All tests now pass.

Only remaining task is the PI public instance type parameter...

@iamdustan iamdustan changed the title from WIP: Fiber test renderer to Fiber test renderer Jan 10, 2017
@@ -463,7 +463,7 @@ module.exports = function<T, P, I, TI, C, CX, CI>(
}
const ref = finishedWork.ref;
if (ref) {
- const instance = getPublicInstance(finishedWork.stateNode);
+ const instance = (getPublicInstance(finishedWork.stateNode) : any);
@iamdustan
iamdustan Jan 10, 2017 Contributor

the flow type for instance on master is any. Whilst adding the PI parameter does end up saying instance is PI, that fails flow due to ref expecting:

ref: null | (((handle : ?Object) => void) & { _stringRef: ?string }),

I suspect this could be improved, but for time constraints at the moment I simply dropped it back to equal the current inference.

constructor(element: ReactElement) {
this._currentElement = element;
this._renderedChildren = null;
this._topLevelWrapper = null;
this._hostContainerInfo = null;
+ this._nodeMock = UNSET;
@gaearon
gaearon Jan 10, 2017 Member

Could we just assign it here instead? We already know the element.

@iamdustan
iamdustan Jan 10, 2017 Contributor

oh, would this need to be invalidated on receiveComponent?

@gaearon
gaearon Jan 10, 2017 Member

Hmm, that's trickier. Probably not since nodes are supposed to be kept intact. I'm actually not sure the API is right now, maybe we should've just passed the type instead of the element. But what's done is done?

@iamdustan
iamdustan Jan 10, 2017 Contributor

welp, I’ve updated it to on mountComponent because that is the earliest point we can do so (that is when hostContainerInfo is passed in)

@gaearon
Member
gaearon commented Jan 10, 2017

Ugh, GH collapsed my comment. Here it is: #8628 (comment).

@gaearon
Member
gaearon commented Jan 10, 2017

This is currently breaking react-test-renderer built package:

screen shot 2017-01-10 at 18 30 33

(No, we don't have automated tests for them.)

This is because ReactDOMFeatureFlags is in dom folder so doesn't become part of the build.

@iamdustan
Contributor

First two thoughts on how to fix this are:

  • add ReactDOMFeatureFlags to the src array of reactTestRenderer in the gulpfile
  • Create a react-test-renderer/fiber.js file for output and do some finagling to continue using ReactDOMFeatureFlags.useFiber for repo unit tests, but not break the build
@gaearon
Member
gaearon commented Jan 10, 2017 edited

I just pushed a commit that explicitly uses the Stack version on npm (since that's what npm package should use).

@iamdustan
Contributor

how do you feel about adding an entrypoint at react-test-renderer/fiber so consumers could opt-in to testing it early, too?

@gaearon
Member
gaearon commented Jan 10, 2017 edited

Added that too in that commit. (But didn't expose it in package.json yet since ReactDOM doesn't expose it either. Not sure if intentional.)

@iamdustan
Contributor

oh yeah. just saw it. 👍

+ } else {
+ var childrenJSON = [];
+ this.children.forEach((child) => {
+ if (typeof child.toJSON === 'function') {
@gaearon
gaearon Jan 10, 2017 Member

When would this ever be false? If I understand correctly child is also either an Instance or TextInstance which we have control over.

@iamdustan
iamdustan Jan 10, 2017 Contributor

disclaimer: this implementation was mostly an amalgamation of ReactNoop and the stack ReactTestRenderer so there is likely some unnecessary complexity from the copypasta.

@gaearon
gaearon Jan 10, 2017 Member

Yea, just saying we should clean this up a little so the next person looking isn't more confused than we were.

+ childrenJSON.push(child.toJSON());
+ }
+ });
+ json.children = childrenJSON.length ? childrenJSON : null;
@gaearon
gaearon Jan 10, 2017 Member

Wouldn't this whole block look simpler as

      json.children = this.children.length ?
        this.children.map(child => child.toJSON()) :
        null;

?

+ this.children = [];
+ this.rootContainerInstance = rootContainerInstance;
+
+ Object.defineProperty(this, 'id', { value: this.id, enumerable: false });
@gaearon
gaearon Jan 10, 2017 Member

What is id field for? I tried removing it everywhere and tests still pass.

+ appendChild(child) {}
+ insertBefore(beforeChild, child) {}
+ removeChild(child) {}
+ toJSON() {}
@gaearon
gaearon Jan 10, 2017 Member

I find these empty implementations odd.
Can you help me understand why they're unnecessary here?

What if I call top-level create with a fragment?

@iamdustan
iamdustan Jan 10, 2017 Contributor

The tldr is I originally wrote the TestContainer to hold the createNodeMock function and keep a pointer to it. This likely can be simplified. Top-level create with a fragment is likely broken right now.

I’ll look at unifying TestComponent and TestContainer tonight. Should I add a test case for top-level create?

@gaearon
gaearon Jan 10, 2017 Member

Maybe. I'm pretty sure them being noops is wrong (since they do get called), but I'm not sure how to write a failing test. If you succeed with making a failing test against this then you'll know how to fix it.

@iamdustan
iamdustan Jan 10, 2017 Contributor

Hmm...this actually seems to maybe work okay already? Here is my current test case:

    it('can update text nodes when rendered as root', () => {
      var Component = (props) => props.children;

      var renderer = ReactTestRenderer.create(<Component>Hi</Component>);
      expect(renderer.toJSON()).toEqual('Hi');
      renderer.update(<Component>{['Hi', 'Bye']}</Component>);
      expect(renderer.toJSON()).toEqual('Hi Bye');
      renderer.update(<Component>Bye</Component>);
      expect(renderer.toJSON()).toEqual('Bye');
      renderer.update(<Component>{42}</Component>);
      expect(renderer.toJSON()).toEqual(42);
      renderer.update(<Component><div /></Component>);
      expect(renderer.toJSON()).toEqual({
        type: 'div',
        children: null,
        props: {},
      });
    });

Though currently <Component>{['Hi', 'Bye']}</Component> toJSON() only returns Hi (instead of Hi Bye). I’m not actually certain what is expected at this point.

I’m going to add another test case later, family-time!

@gaearon
gaearon Jan 10, 2017 Member

I'm confused. The test you're showing looks like my test about text nodes. But we were discussing root in this thread.

@iamdustan
iamdustan Jan 10, 2017 Contributor

Sorry, this is the root node test as I understand it: https://github.com/facebook/react/pull/8628/files#diff-417ca2789067ee4dcafab55a48aa03cbR532

    it('can render and update root fragments', () => {
      var Component = (props) => props.children;

      var renderer = ReactTestRenderer.create([
        <Component>Hi</Component>,
        <Component>Bye</Component>,
      ]);
      expect(renderer.toJSON()).toEqual('Hi');
      renderer.update(<div />);
      expect(renderer.toJSON()).toEqual({
        type: 'div',
        children: null,
        props: {},
      });
      renderer.update([<Component>{42}</Component>, <Component>The answer</Component>]);
      expect(renderer.toJSON()).toEqual(42);
    });
@gaearon
gaearon Jan 11, 2017 Member

Yea, I don't really know how to trigger the failing case for containers. It's just odd to me that those methods are blank. I would expect that either they shouldn't be defined at all, or they should do something.

@gaearon
gaearon Jan 11, 2017 Member

expect(renderer.toJSON()).toEqual('Hi');

I don't think this looks right. We are rendering an array. Why do we get a single item as an output? I would expect it to return an array in Fiber.

@iamdustan
iamdustan Jan 11, 2017 Contributor

https://github.com/facebook/react/blob/e4733076498695c754740ff3a8f9d41c447b1a3e/src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js#L137-L158

The underlying implementation uses TestRenderer.findHostInstance(root). From looking at what I believe are the corresponding DOM tests, only the first element in a fragment is returned.

+ },
+
+ resetTextContent(testElement : Instance) : void {
+ // Noop
@gaearon
gaearon Jan 10, 2017 Member

Why is this a no-op? It seems like there could be bugs related to this.
(e.g. DOM renderer implements this one.)

I think we should actually have shouldSetTextContent always return false.
AFAIK it only exists as an optimization to avoid creating additional Fibers for single-text-child divs.
But in case of test renderer it doesn't matter.

@iamdustan
iamdustan Jan 10, 2017 Contributor

what is the tdlr of this method? No tests currently require it so I just left it empty. Sounds like I’ll need to add a test case...

@gaearon
gaearon Jan 10, 2017 edited Member

Try commenting its implementation out in DOM renderer and see what fails.
By grepping for method name, I found this:

      if (nextEffect.effectTag & ContentReset) {
        config.resetTextContent(nextEffect.stateNode);
      }

By grepping for ContentReset, I found this:

      // If we're switching from a direct text child to a normal child, or to
      // empty, we need to schedule the text content to be reset.
      workInProgress.effectTag |= ContentReset;

So it likely happens when we switch from a "direct text child" (with your implementation of shouldSetTextContent, it's the case with <div>Text</div> or <div>{42}</div>) to a "normal child" (e.g. <div><p /></div>). The purpose is to clear the text in this case.

I think you can avoid the whole problem by making shouldSetTextContent always return false. Then this code never runs and we always create a real text instance.

@gaearon
gaearon Jan 10, 2017 Member

Here is a failing test:

  it('can update text nodes', () => {
    class Component extends React.Component {
      render() {
        return (
          <div>
            {this.props.children}
          </div>
        );
      }
    }

    var renderer = ReactTestRenderer.create(<Component>Hi</Component>);
    expect(renderer.toJSON()).toEqual({
      type: 'div',
      children: ['Hi'],
      props: {},
    });
    renderer.update(<Component>{['Hi', 'Bye']}</Component>);
    expect(renderer.toJSON()).toEqual({
      type: 'div',
      children: ['Hi', 'Bye'],
      props: {},
    });
    renderer.update(<Component>Bye</Component>);
    expect(renderer.toJSON()).toEqual({
      type: 'div',
      children: ['Bye'],
      props: {},
    });
    renderer.update(<Component>{42}</Component>);
    expect(renderer.toJSON()).toEqual({
      type: 'div',
      children: [42],
      props: {},
    });
    renderer.update(<Component><div /></Component>);
    expect(renderer.toJSON()).toEqual({
      type: 'div',
      children: [{
        type: 'div',
        children: null,
        props: {},
      }],
      props: {},
    });

Solved by returning false from shouldSetTextContent.

+ }
+ var container = new TestContainer(createNodeMock);
+ var root = TestRenderer.createContainer(container);
+ if (root) {
@gaearon
gaearon Jan 10, 2017 Member

I don't see Flow errors removing this condition.

@iamdustan
iamdustan Jan 10, 2017 Contributor

great! sounds like it’s been magically resolved :)

@iamdustan
iamdustan Jan 10, 2017 Contributor

hmm...I can remove this one, but not those in the returned methods...

@iamdustan
iamdustan Jan 10, 2017 Contributor

oh duh. It’s because in unmount we clean up our closured reference to root, but the consumer could still do:

const renderer = TestRenderer.create(<Comp />);
renderer.unmount();
renderer.update(<OtherComp />); // shouldn’t be possible because we unmounted.

I’ll need to look into what the current implementation does...

+ return;
+ }
+ TestRenderer.updateContainer(null, root, null, () => {
+ container = null;
@gaearon
gaearon Jan 10, 2017 Member

I think you shouldn't need this callback.
Since it uses synch scheduling, everything should happen synchronously anyway.
So you should be able to clean up immediately after calling updateContainer.

+ var renderer = ReactTestRenderer.create(<Component>Hi</Component>);
+ expect(renderer.toJSON()).toEqual('Hi');
+ renderer.update(<Component>{['Hi', 'Bye']}</Component>);
+ expect(renderer.toJSON()).toEqual('Hi');
@iamdustan
iamdustan Jan 10, 2017 Contributor

I’m not actually certain if this should be Hi or Hi Bye. I think that when given a fragment-like thing the first is the only one returned, but only spent 30 seconds on it so far.

@gaearon
gaearon Jan 11, 2017 Member

It should be an array in Fiber. We should put the whole test behind a feature flag because it is Fiber specific. I think this demonstrates that root problem but maybe I'm wrong.

iamdustan and others added some commits Dec 22, 2016
@iamdustan iamdustan ReactTestRenderer move current impl to stack dir 29ac145
@iamdustan iamdustan ReactTestRenderer on fiber: commence! 1398f76
@iamdustan iamdustan ReactTestRenderer: most non-ref/non-public-instance tests are passing 791358c
@iamdustan iamdustan Move ReactTestFiberComponent functions from Renderer to Component file 12bdc91
@iamdustan iamdustan test renderer: get rid of private root containers and root Maps 07964e0
@iamdustan iamdustan TestRenderer: switch impl based on ReactDOMFeatureFlag.useFiber 44a3ebe
@iamdustan iamdustan ReactTestRenderer: inline component creation 92df27f
@iamdustan iamdustan ReactTestRenderer: return to pristine original glory (+ Fiber for err…
…or order difference)
ba70bcd
@iamdustan iamdustan TestRendererFiber: use a simple class as TestComponentInstances 282f346
@iamdustan iamdustan Add `getPublicInstance` to support TestRenderer `createNodeMock` 20efda4
@iamdustan iamdustan Rename files to end. Update for `mountContainer->createContainer` change da31fec
@iamdustan iamdustan test renderer return same object to prevent unnecessary context pushi…
…ng/popping
1f1c536
@iamdustan iamdustan Fiber HostConfig add getPublicInstance. This should be the identity f…
…n everywhere except the test renderer
9e2615c
@iamdustan iamdustan appease flow b2f7c3e
@iamdustan iamdustan Initial cleanup from sleepy work fdfbafe
@iamdustan iamdustan unstable_batchedUpdates 3d3fbce
@iamdustan iamdustan Stack test renderer: cache nodeMock to not call on unmount 66e61bb
@iamdustan iamdustan add public instance type parameter to the reconciler e172128
@iamdustan iamdustan test renderer: set _nodeMock when mounted c4bacd9
@iamdustan iamdustan More cleanup ebce5cb
@iamdustan iamdustan Add test cases for root fragments and (maybe?) root text nodes 33f9365
@gaearon @iamdustan gaearon Fix the npm package build
Explicitly require the Stack version by default.
Add a separate entry point for Fiber.

We don't add fiber.js to the package yet since it's considered internal until React 16.
f481718
@gaearon @iamdustan gaearon Relax the ref type from Object to mixed
This seems like the most straightforward way to support getPublicInstance for test renderer.
2af0a17
@gaearon @iamdustan gaearon Remove accidental newline 08ab18f
@iamdustan iamdustan test renderer: unify TestComponent and TestContainer, handle root upd…
…ates
7347a5e
@iamdustan iamdustan Remove string/number serialization attempts since Fiber ensures all t…
…extInstances are strings
05ac531
@iamdustan iamdustan Return full fragments in toJSON e8a13aa
@iamdustan
Contributor
iamdustan commented Jan 11, 2017 edited

Okay, I can get rid of all the new container instances for simple objects in exchange for this toJSON function. The reason this is still necessary is because:

  • text instances need to be an object to be updated (commitTextUpdate(instance, oldText, newText) mutates the instance)
  • need to be able to ignore the props in children {props: {children: JSXProvidedChildrenWhichAreFibers}}

This is the toJSON function:

const toJSON = child => {
  if (child.text) {
    return child.text;
  }

  /* eslint-disable no-unused-vars */
  /* ignoring the children prop and managing that ourselves*/
  const {children, ...props} = child.props;
  /* eslint-enable */
  const json = {
    type: child.type,
    props: props,
    children: null,
  };
  Object.defineProperty(json, '$$typeof', {value: Symbol.for('react.test.json')});
  if (child.children) {
    json.children = child.children.length
      ? child.children.map(toJSON)
      : null;
  }
  return json;
};

I can make that happen if you want. Flow is very unhappy with me at the moment, though so 🤷‍♂️

@gaearon
Member
gaearon commented Jan 11, 2017

I can make that happen if you want.

If it looks/works better, sure. If not, don’t bother. :-)
I’m not sure I fully understand the issues so it’s hard for me to say yes or no without seeing it.

@iamdustan
Contributor

WIP commit d228c24 just pushed. I have three more flow issues to resolve to finish it off or I can revert it if it’s okay as is.

@gaearon
Member
gaearon commented Jan 11, 2017

If you can figure out how to resolve them that would be great. Otherwise I can try to take a look.
I want to get this in today, in one way or another.

@iamdustan
Contributor

👍 on it now.

- parentInstance.removeChild(child);
+ const index = parentInstance.children.indexOf(child);
+ if (index === -1) {
+ throw new Error('This child does not exist.');
@gaearon
gaearon Jan 11, 2017 Member

Note sure those checks are important. They would indicate bugs in Fiber.
They exist in ReactNoop because it tests Fiber itself but doesn't look necessary here.

@@ -246,8 +193,18 @@ var TestRenderer = ReactFiberReconciler({
useSyncScheduling: true,
getPublicInstance(ref) {
- const createNodeMock = ref.rootContainerInstance.createNodeMock;
- return createNodeMock(ref);
+ if (typeof ref.text === 'string') {
@gaearon
gaearon Jan 11, 2017 Member

Can we use a $$typeof switch here instead?

@iamdustan
iamdustan Jan 11, 2017 Contributor

how would that look? I could add $$typeof symbols to the container and text instances..

+ }
+
+ // this should not be possible
+ throw new Error('Attempted to getPublicInstance on an invalid ref.');
@gaearon
gaearon Jan 11, 2017 Member

Hmm. Shouldn't ref be guaranteed to be either Instance or Container?

@iamdustan
iamdustan Jan 11, 2017 Contributor

I thought it was just Instance or TextInstance, though Container is also actually a possibility.

This was added for exhaustiveness because I couldn’t get flow to be okay without else if check just before this.

@gaearon
gaearon Jan 11, 2017 Member

Okay. Switch on $$typeof with a throwing default would look nicer than duck-typing though.

@iamdustan
iamdustan Jan 11, 2017 Contributor

are there any examples of type checking with symbols?

@gaearon
gaearon Jan 11, 2017 Member

Not really important to use symbols there (if you don't leak those objects externally), you can just use an enum. Example declaration and switch.

@iamdustan
iamdustan Jan 11, 2017 Contributor

I added static strings before seeing this message. Okay? 799e901

@iamdustan
Contributor

I would like to take a brief moment to say thank you for your infinite patience and attention to detail in this PR @gaearon. 🥇

@@ -28,7 +32,8 @@ type ReactTestRendererJSON = {|
type Container = {|
children: Array<Instance | TextInstance>,
- createNodeMock: Function
+ createNodeMock: Function,
+ $$typeof: typeof CONTAINER_TYPE,
@gaearon
gaearon Jan 11, 2017 Member

I think this could be just $$typeof: CONTAINER_TYPE?
Same below.

@iamdustan
iamdustan Jan 11, 2017 Contributor

without the typeof I get the following:

src/renderers/testing/ReactTestRendererFiber.js:36
 36:   $$typeof: CONTAINER_TYPE,
                 ^^^^^^^^^^^^^^ string. Ineligible value used in/as type annotation (did you forget 'typeof'?)
 36:   $$typeof: CONTAINER_TYPE,
                 ^^^^^^^^^^^^^^ CONTAINER_TYPE
@gaearon
Member
gaearon commented Jan 11, 2017

This looks good to me. Thanks for sticking with it. 😄
I’ll merge later today.

+ ) : void {
+ const index = parentInstance.children.indexOf(child);
+ if (index !== -1) {
+ this.children.splice(index, 1);
@gaearon
gaearon Jan 11, 2017 Member

What is this.children?

@iamdustan
iamdustan Jan 11, 2017 Contributor

a bug. should be parentInstance.children.

@gaearon gaearon added this to the 15-lopri milestone Jan 11, 2017
@gaearon gaearon merged commit 2da35fc into facebook:master Jan 11, 2017

1 check passed

ci/circleci Your tests passed on CircleCI!
Details
@gaearon
Member
gaearon commented Jan 11, 2017

Congratulations. 😄

@iamdustan iamdustan deleted the iamdustan:fiber-test-renderer branch Jan 11, 2017
@@ -462,7 +463,7 @@ module.exports = function<T, P, I, TI, C, CX>(
}
const ref = finishedWork.ref;
if (ref) {
- const instance = finishedWork.stateNode;
+ const instance = getPublicInstance(finishedWork.stateNode);
@sebmarkbage
sebmarkbage Jan 11, 2017 Member

This is not type safe for the case finishedWork.tag === ClassComponent.

@iamdustan
iamdustan Jan 11, 2017 Contributor

how so? what can I do to rectify that?

@sebmarkbage
sebmarkbage Jan 11, 2017 Member

finishedWork.stateNode on a a class component will be the instance of the class. That doesn't match the generic types I | TI. I think it only accidentally works on runtime now because you assume they won't have a .tag field with the string INSTANCE so they fallback.

You can make separate branches by switching on finishedWork.tag and only use getPublicInstance on the branch on HostComponent.

+
+ useSyncScheduling: true,
+
+ getPublicInstance(inst) {
@sebmarkbage
sebmarkbage Jan 11, 2017 Member

Hm. It's strange that you're allowed to avoid the type annotation here. It probably leads to some overly optimistic analysis by Flow. We should add a type annotation here. It's important to do in the renderers since otherwise the generic arguments can be inferred to be very weak.

@iamdustan
iamdustan Jan 11, 2017 Contributor

Related, how can I type the createNodeMock function so PI is properly inferred?

@iamdustan iamdustan added a commit to iamdustan/react that referenced this pull request Jan 11, 2017
@iamdustan iamdustan Fix ReactFiberReconciler annotation to include `PI` 9d42ea0
@iamdustan iamdustan added a commit to iamdustan/react that referenced this pull request Jan 11, 2017
@iamdustan iamdustan Fix ReactFiberReconciler annotation to include `PI` 2b89f90
@iamdustan iamdustan referenced this pull request in arcanis/ohui-v2 Jan 12, 2017
Merged

Return same emptyObject #1

@iamdustan iamdustan added a commit to iamdustan/react that referenced this pull request Jan 17, 2017
@iamdustan iamdustan Fix ReactFiberReconciler annotation to include `PI` 39a9cc7
@iamdustan iamdustan added a commit to iamdustan/react that referenced this pull request Jan 17, 2017
@iamdustan iamdustan Fix ReactFiberReconciler annotation to include `PI` 48a92e0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment