Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add hooks support to ReactShallowRenderer #14567

Merged
merged 7 commits into from Jan 17, 2019

Conversation

@trueadm
Copy link
Member

@trueadm trueadm commented Jan 10, 2019

This PR adds React Hooks support to React's test Shallow Renderer. The implementation is mostly based on the ReactDOMServer hooks dispatcher implementation, with a few tweaks to account for the fact that shallow renderer instances do not share state. The ReactDOMServer implementation can be found here:

https://github.com/facebook/react/blob/master/packages/react-dom/src/server/ReactPartialRendererHooks.js#L340-L354

I've added a selection of tests too that confirm the expected behaviour works as intended.

@trueadm trueadm changed the title Add hook support to ReactShallowRenderer Add hooks support to ReactShallowRenderer Jan 10, 2019
@facebook facebook deleted a comment from sizebot Jan 10, 2019
@sizebot
Copy link

@sizebot sizebot commented Jan 10, 2019

Details of bundled changes.

Comparing: 1454a8b...4bc5250

react-test-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-test-renderer.development.js 0.0% 0.0% 455.59 KB 455.59 KB 98.36 KB 98.37 KB UMD_DEV
react-test-renderer.production.min.js 0.0% -0.0% 57.35 KB 57.35 KB 17.62 KB 17.62 KB UMD_PROD
react-test-renderer.production.min.js 0.0% -0.0% 57.02 KB 57.02 KB 17.46 KB 17.46 KB NODE_PROD
ReactTestRenderer-dev.js 0.0% 0.0% 459.14 KB 459.14 KB 96.51 KB 96.51 KB FB_WWW_DEV
react-test-renderer-shallow.development.js +48.8% +42.0% 25.67 KB 38.2 KB 6.95 KB 9.87 KB UMD_DEV
react-test-renderer-shallow.production.min.js 🔺+45.3% 🔺+37.6% 7.31 KB 10.63 KB 2.38 KB 3.28 KB UMD_PROD
react-test-renderer-shallow.development.js +62.8% +54.4% 19.97 KB 32.5 KB 5.51 KB 8.5 KB NODE_DEV
react-test-renderer-shallow.production.min.js 🔺+41.7% 🔺+33.5% 7.96 KB 11.27 KB 2.64 KB 3.52 KB NODE_PROD
ReactShallowRenderer-dev.js +72.3% +68.8% 17.93 KB 30.89 KB 4.64 KB 7.83 KB FB_WWW_DEV

Generated by 🚫 dangerJS

@bvaughn bvaughn self-assigned this Jan 15, 2019
Copy link
Contributor

@bvaughn bvaughn left a comment

This mostly looks to match up with the existing partial renderer's implementation.

I'm a little concerned about us forgetting to update one if we make changes to the other though.

@gaearon
Copy link
Member

@gaearon gaearon commented Jan 16, 2019

Seems like this leaks state if you render different component types one after another.

Failing test:

  it('should not leak state when component type changes', () => {
    function SomeComponent({defaultName}) {
      const [name] = React.useState(defaultName);

      return (
        <div>
          <p>
            Your name is: <span>{name}</span>
          </p>
        </div>
      );
    }

    function SomeOtherComponent({defaultName}) {
      const [name] = React.useState(defaultName);

      return (
        <div>
          <p>
            Your name is: <span>{name}</span>
          </p>
        </div>
      );
    }

    const shallowRenderer = createRenderer();
    let result = shallowRenderer.render(
      <SomeComponent defaultName={'Dominic'} />,
    );
    expect(result).toEqual(
      <div>
        <p>
          Your name is: <span>Dominic</span>
        </p>
      </div>,
    );

    result = shallowRenderer.render(
      <SomeOtherComponent defaultName={'Dan'} />,
    );
    expect(result).toEqual(
      <div>
        <p>
          Your name is: <span>Dan</span>
        </p>
      </div>,
    );
  });
Copy link
Member

@gaearon gaearon left a comment

I think overall this looks right but a few things need to change:

  • We should only set the dispatcher before rendering, and restore it after. try / finally like we do in SSR.
  • Don't leak state between components (#14567 (comment)). Rendering a different component type or key should destroy Hook state, like it happens with classes.
  • We need to apply changes from #14569 and #14594 for consistency. For example #14569 (comment). (oops, not this one)
Copy link
Contributor

@bvaughn bvaughn left a comment

Also tag the return value of _createDispatcher to the typeof Dispatcher (now that #14599 has landed) 😄

@trueadm
Copy link
Member Author

@trueadm trueadm commented Jan 17, 2019

I've made all the relevant changes, plus added Flow to the file. I wasn't sure about applying #14569 as per @gaearon's feedback above. The server side partial renderer doesn't have the same logic applied to it, so maybe I'll wait on feedback from @acdlite as to if we need it here too.

this._createWorkInProgressHook();

const nextInputs =
inputs !== undefined && inputs !== null ? inputs : [nextCreate];

This comment has been minimized.

@gaearon

gaearon Jan 17, 2019
Member

To match #14594, let's call them deps (not inputs) and make sure we use null rather than [nextCreate] in all cases. (That's what #14594 was about.)

This comment has been minimized.

@trueadm

trueadm Jan 17, 2019
Author Member

I can do that, but the reason I didn't was because #14594 didn't make that change to packages/react-dom/src/server/ReactPartialRendererHooks.js.

This comment has been minimized.

@gaearon

gaearon Jan 17, 2019
Member

I think that's just an omission.

Copy link
Member

@gaearon gaearon left a comment

Looks good but let's fix the [nextCreate]s to be nulls

@trueadm
Copy link
Member Author

@trueadm trueadm commented Jan 17, 2019

@gaearon I made that change but it broke things. I don't believe the same logic is meant to be here without changing the types for areHookInputsEqual. Given this function was already changed in #14599 to have these flow types, I believe this to be intentional. I've made the input -> deps rename though.

@gaearon
Copy link
Member

@gaearon gaearon commented Jan 17, 2019

I'm not sure I fully understand what you're saying.

What I'm saying is — since #14594, useMemo and useCallback doesn't compare the function itself. Previously they considered it as part of deps but not anymore. We need to ensure this behavior is replicated in all implementations. I haven't looked exactly at how to do it (maybe it's more than changing that one line) but it needs to be done for semantics to match.

@Jessidhia
Copy link
Contributor

@Jessidhia Jessidhia commented Jan 17, 2019

FWIW, just as useCallback used to be just an expensive identity function when called without deps, useMemo without deps array is now just an expensive .call() (it was before too, but there was one corner case where it wouldn't be; that corner case is now gone).

I think the deps argument should be required for them.

I should make an issue about it when I'm not half-asleep... 😴

@trueadm
Copy link
Member Author

@trueadm trueadm commented Jan 17, 2019

@gaearon I've made that change. I was more suggesting there was some confusion over the referenced PR – as it didn't apply the same changed to the partial renderer and I assumed that this was intentional.

@gaearon
Copy link
Member

@gaearon gaearon commented Jan 17, 2019

lgtm

This was referenced Sep 20, 2019
NMinhNguyen referenced this pull request in enzymejs/react-shallow-renderer Jan 29, 2020
* Add hook support to ReactShallowRenderer
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

10 participants