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

Support context components in react-test-renderer #12151

Closed
wants to merge 2 commits into from
Closed

Support context components in react-test-renderer #12151

wants to merge 2 commits into from

Conversation

pshrmn
Copy link

@pshrmn pshrmn commented Feb 4, 2018

Potential fix for #12150. I had to use the pendingProps instead of memoizedProps for the ContextConsumer (I don't know enough about React's internals to know why that doesn't have a memoizedProps).

@gaearon
Copy link
Collaborator

gaearon commented Feb 4, 2018

I think we shouldn't be using pendingProps. Maybe type.value is enough there.

That I said I'm not convinced that context providers and consumers need to be in the output at all. What's the point of asserting on them if they don't contain their own logic?

I think maybe we should just skip over them.

@pshrmn
Copy link
Author

pshrmn commented Feb 5, 2018

If I understand you correctly, ContextConsumer and ContextProvider should be treated somewhat like a Fragment? I see two places where Fragments are used here:

  1. In toTree they just pass on their child Fiber's tree instead of returning themselves.
  2. When getting the children of an element (for querying with the find___ methods), the Fragment will skip to its child. This means that the find___ methods will never match a Fragment.

I think the first definitely makes sense to replicate with the ContextConsumer and ContextProvider. The second probably makes sense, but I'm not as confident there.

There is also one issue that I noticed. While using the children getter works for ignoring nested elements, the root will still be matched by the find___ methods. This isn't an issue with Fragments because reconcileChildFibers bypasses a Fragment.

// if root.children does not return ContextConsumers and ContextProviders
const { Provider, Consumer } = React.createContext('waldo');

// this works
const inst1 = renderer.create(
  <Provider value='waldo'>
  </Provider>
);
const p1 = inst.root.findByType(Provider);

// this throws
const inst2 = renderer.create(
  <div>
    <Provider value='waldo'>
    </Provider>
  </div>
);
const p2 = inst.root.findByType(Provider);

@acdlite acdlite mentioned this pull request Feb 5, 2018
28 tasks
@pshrmn
Copy link
Author

pshrmn commented Feb 6, 2018

I rebased after #12154, but I'm still not positive exactly what the behavior should be, and since I know you want to get 16.3.0 out soon, it might make more sense for someone on the React team to make these changes instead.

@pshrmn
Copy link
Author

pshrmn commented Feb 16, 2018

#12237

@pshrmn pshrmn closed this Feb 16, 2018
@pshrmn pshrmn deleted the test-renderer-context branch February 16, 2018 19:43
@acdlite
Copy link
Collaborator

acdlite commented Feb 16, 2018

@pshrmn Shoot, I didn't see this PR before opening #12237. I'm sorry :(

@pshrmn
Copy link
Author

pshrmn commented Feb 17, 2018

@acdlite No worries, just glad to see a solution. 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants