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

New hooks api suggestion / discussion -- inspectHook() and setHookState() #2120

Open
chenesan opened this issue May 10, 2019 · 5 comments
Open
Projects

Comments

@chenesan
Copy link
Contributor

chenesan commented May 10, 2019

With current enzyme api, usually we have to get / set hook state from rendered output (e.g. print state in rendered div / setCount on button). it's hard to check and change current hook state value through enzyme API (like .state() and .setState() for class component.). Though some (or lots of ?) people may think it's testing implementation detail, I think it's still valuable to have an api to get / set the result of hook state.

There are two new apis I'd like to suggest. Hope to get some more feedbacks :-)

inspectHook()

The first is inspectHook() -- an api to get the whole hook stack tree in current function component node. In fact inspectHookWithFiber() is an api name from the react-debug-tool in React's internal packages for hook testing, and I'd like to port this module into enzyme.

With inspectHookWithFiber() then we can get the whole hook stack tree in a render function and write test like this. The tree currently has the following structure

// from source code of above link
type HooksNode = {
  id: number | null,
  isStateEditable: boolean,
  name: string,
  value: mixed,
  subHooks: Array<HooksNode>,
};

we can get the state from value, hook function name from name, and subHooks (sub hooks called in this hook) in subHooks.

In fact in react-devtool they also get current hook state value with it after some tweaks.

use cases

Basically the use cases would be similar to the tests in react-debug-tools. The following use case is an example of inspectHook() in enzyme; For such simple test case the better way to test the counter may be just find() the <p> and assert the count, though. :

function Counter() {
  const [count, setCount] = React.useState(0);
  const increment = () => setCount(prevCount => prevCount + 1);;
  return (
    <div>
      <button onClick={increment} />
      <p>{count}</p>
    <div>
  );
}

const wrapper = mount(<Counter />);
const tree = wrapper.inspectHook();
expect(tree[0].value).toEqual(0);
ReactTestUtils.act(() => wrapper.find('button').prop('onClick')());
const nextTree = wrapper.inspectHook();
expect(nextTree[0].value).toEqual(1);

Note that not only useState() will be in the tree returned from inspectHook(); All hooks called in the function component will be logged, if it's a hook with state the value will be the state, or it would depends on what the hook is.

limitation

With this way we can get the state of hook, but we cannot get the returned value of hook calls. So it's still not possible to get the seState returned from useState(), or dispatch() from useReducer(), or returned value from custom hook in this way.

Also I'm not sure if it's doable in shallow() since shallow renderer is not based on fiber.

implementation

  1. given a wrapper of single functional component node, find its relative fiber node.
  2. pass it into inspectHookOfFiber() which we bring into enzyme and return the result.

As for porting inspectHookOfFiber(), we have to get the hook dispatcher in react secret shared internals in enzyme. In react-devtool they have their own way to communicate with React (React in fact inject the secret internal into the react-devtool to make it work). I'm sure we can get the secret internal through React.__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED.
Though it works to use it directly, It's not a good idea to couple with that private internal. The alternative way may be add devtool hook in enzyme side and let the React inject the internal into enzyme. I'll try to look into this further more.

setHookState()

The second is setHookState() -- an api to set the state of hook and trigger the rerender. Like inspectHook() there's also an internal function in React called overrideHookState, which is injected into react-devtools here so you can edit the state value of hook in react-devtool. I'd also like to port this functionality into enzyme, too.

use case

The usage of overrideHookState in React is like this

    function Counter() {
      const [count, setCount] = React.useState(0);
      const increment = () => setCount(prevCount => prevCount + 1);;
      return (
        <div>
          <button onClick={increment} />
          <p>{count}</p>
        <div>
      );
    }

    const renderer = ReactTestRenderer.create(<MyComponent />);
    expect(renderer.toJSON()).toEqual({
      type: 'div',
      props: {},
      children: ['count:', '0'],
    });

    const fiber = renderer.root.findByType(MyComponent)._currentFiber();
    const tree = ReactDebugTools.inspectHooksOfFiber(fiber);
    const stateHook = tree[0];
    expect(stateHook.isStateEditable).toBe(true);

    overrideHookState(fiber, stateHook.id, [], 10);
    expect(renderer.toJSON()).toEqual({
      type: 'div',
      props: {},
      children: ['count:', '10'],
    });

In enzyme we could find the fiber and handle the state update path (the 3rd arg) by ourselves. So the api only need to receive 2 arguments: id for the hook id, which is included in the tree node returned from inspectHook, and value for the changed value:

    function Counter() {
      const [count, setCount] = React.useState(0);
      const increment = () => setCount(prevCount => prevCount + 1);;
      return (
        <div>
          <button onClick={increment} />
          <p>{count}</p>
        <div>
      );
    }

    const wrapper = mount(<Counter />);
    const tree = wrapper.inspectHook();
    const stateHook = tree[0];
    expect(stateHook.value).toBe(0);

    wrapper.setHookState(stateHook.id, 10);

    expect(wrapper.find('p').text()).toEqual('10');

implementation

To port the overrideHookState it looks like the only way is to let React inject devtool internal into it. I think we could build a same global hook as react-devtool before import React, but I haven't tried it yet nor known if we can always ensure React injects internal after we install the hook.

If it's doable then the remaining work is as same as inspectHook -- find the fiber node and call overrideHookState to get the result.

Planning

If these sound good, I'd like to work on this with following steps:

  1. Port inspectHookOfFiber into enzyme, with hard-coding secret internal (i.e. directly use React.__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED), and prove that we are able to add .inspectHook() in enzyme.
  2. Try to add a devtool hook in enzyme so we don't need the hard coding internal.
  3. if (2) works then building setHookState() should be relatively easy.

Also note that facebook/react#14906 (comment) said that react-debug-tools should be released soon. Once it's released we can just use inspectHookOfFiber() without porting (But might still need to build a devtool hook for overrideHookState). I'll also open an issue to request the releasing if new apis are desirable. It also might be good to start work after React releasing this.

And I'm not sure if it's what enzyme maintainers / users want. So feedback are welcome :-)

Related old discussion / PR in React / react-devtool: facebook/react-devtools#1272 facebook/react#14906

@ljharb ljharb added this to Needs Triage in React 16 via automation May 10, 2019
@ljharb
Copy link
Member

ljharb commented May 10, 2019

(related to #2011)

Thanks for filing this; this is as good a place as any to discuss it.

I think that before any hook-specific APIs are added to enzyme, we'd need to finish shipping full hooks support in existing APIs - ie, wrapping components with hooks should work as you'd expect them to work.

@chenesan
Copy link
Contributor Author

Thanks for reply @ljharb . I agree that hooks supports for existing API is important :-) I'd also be back to finish remaining tests in #2011 recently.

And I saw people (e.g. #2012 ) try to set state of hooks with enzyme. I guess some people (include me) test hooks with enzyme even though enzyme doesn't officially support hooks and expect some ways to get / set hooks more directly. So maybe it's still worth to discuss this even when we haven't finished all the hooks tests / bugfix for existing apis. I'd like to get some feedback around the proposal and just curious to know if you have any opinion on that :-)

@ljharb ljharb moved this from Needs Triage to v16.8+: Hooks in React 16 May 11, 2019
@ljharb
Copy link
Member

ljharb commented May 11, 2019

If it is actually possible to get at the hook information equally in both shallow and mount, at any level in the tree, that seems interesting. However, I'd want to get the opinion of the react core team before adding such an API - since if react 17 is going to make it impossible to get this info, it's not worth adding now.

@chenesan
Copy link
Contributor Author

chenesan commented May 12, 2019

Looks like there are 2 things we could do:

  1. File an issue in React to track the release of react-denug-tools -- If they'll publish this, then we can make sure we can leverage inspectHook safely. Also we can try to ask for including overrideHookState in react-debug-tools, or at least make sure we can safely use it through devtool hook.

  2. Look into if we could also get / set hook state in shallow renderer in other way.

I'll open an issue in React today or tomorrow :-)

(I'd also work on #2011 too. However because of some personal health issue I'm not able to code for 1 or 2 weeks. Hope to be back soon :-)

(updated: issue filed facebook/react#15624)

@chenesan
Copy link
Contributor Author

According to [react core team's reply(https://github.com/facebook/react/issues/15624#issuecomment-491649500) it looks like that:

  1. They're planning to release react-debug-tools, but it's still possible to be private.
  2. There's no plan to provide any way to set hook state. Devtool hook is for devtool only.

So maybe in the future it is possible to have inspectHook; There should be no safe way to build setHookState since we could not leverage overrideHookState.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
React 16
  
v16.8+: Hooks
Development

No branches or pull requests

2 participants