Enables finding nodes created by forwardRef #2
Conversation
| @@ -113,11 +113,21 @@ function toTree(vnode) { | |||
| } | |||
| case HostText: // 6 | |||
| return node.memoizedProps; | |||
| case ForwardRef: { | |||
There was a problem hiding this comment.
This is the crux of the change. I realise that this toTree function is largely adapted from toTree in react-test-renderer. This creates a divergence in behaviour between the two libraries which sounds scary. Ideally, this change would go into react-test-renderer and then enzyme could use the toTree function directly.
| return { | ||
| nodeType: 'function', | ||
| type: node.type, | ||
| props: { ...node.pendingProps }, |
There was a problem hiding this comment.
why pendingProps? is that all that's there on this fiber?
There was a problem hiding this comment.
Please don't use pendingProps anywhere.
In fact all of the fields Enzyme uses are subject to breakage. But I guess we talked about this before.
There was a problem hiding this comment.
Yeah I was really just feeling around in the dark here. I used pendingProps because that looked like it had the information I needed. Is it any worse than memoizedProps that the other tags use?
There was a problem hiding this comment.
pendingProps get cleared by React after rendering. So yes, it's worse and shouldn't be read.
| @@ -113,11 +113,21 @@ function toTree(vnode) { | |||
| } | |||
| case HostText: // 6 | |||
| return node.memoizedProps; | |||
| case ForwardRef: { | |||
| return { | |||
| nodeType: 'function', | |||
There was a problem hiding this comment.
hmm we should probably have a new type for this, but that would require much wider enzyme changes cc @aweary I think this is ok for now?
| // The node type can be defined in three different places. | ||
| // Where the type is depends on the node and rendering strategy. | ||
| // This function looks in the three spots and returns the type. | ||
| const getNodeType = (node) => { |
There was a problem hiding this comment.
I think you can use typeOf from react-is for this, no?
There was a problem hiding this comment.
I think i tried that and it didn't work...the node here isn't an element, which i think is what is expected by react-is
|
@jquense has there been any attempts to make an adapter based on |
ljharb
force-pushed
the
forward-ref
branch
3 times, most recently
from
62a06f6 to
f818a17
Compare
ljharb
force-pushed
the
forward-ref
branch
3 times, most recently
from
f0c7b6a to
6be05e9
Compare
ljharb
force-pushed
the
forward-ref
branch
2 times, most recently
from
7fc51c1 to
d7a4ab3
Compare
ljharb
force-pushed
the
forward-ref
branch
3 times, most recently
from
b90bc58 to
965b785
Compare
Hi mate, I have made some further changes based on your initial work to support
forwardRef.While I was able to render
forwardRefnodes in my tests, I ran into problems when trying to find those components. This is becauseforwardRefnodes are skipped in thetoTreemethod. I explained the problem in more detail on this issue.These changes make
forwardRefnodes look more similar to functional or class-based react nodes.We currently have a large codebase using Enzyme, so hopefully tomorrow I'll get a chance to test this fork out against that codebase.