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)

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.

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

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

Comments

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

Are pushing undefined? Why?

Please clean up the comments.

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.

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.

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.

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

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.

When is it null?

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

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.

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

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

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

😇

...adding this to the PR checklist.

Please do.

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?

outdated comment.

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

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)

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

oh, would this need to be invalidated on receiveComponent?

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?

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)

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

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

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

Wouldn't this whole block look simpler as

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

?

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

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?

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?

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.

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!

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

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);
    });

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.

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.

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.

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.

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

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.

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.

I don't see Flow errors removing this condition.

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

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

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

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.

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.

Can we use a $$typeof switch here instead?

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

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

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.

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

are there any examples of type checking with symbols?

Like this?

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.

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

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

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

What is this.children?

a bug. should be parentInstance.children.