Skip to content

Forward ref#1738

Merged
ljharb merged 6 commits intoenzymejs:masterfrom
suchipi:forward-ref
Aug 16, 2018
Merged

Forward ref#1738
ljharb merged 6 commits intoenzymejs:masterfrom
suchipi:forward-ref

Conversation

@suchipi
Copy link
Copy Markdown
Contributor

@suchipi suchipi commented Aug 8, 2018

Rebased version of #1592 with test added for the special proptype handling and failing tests fixed.

Fixes #1604.

@suchipi suchipi mentioned this pull request Aug 8, 2018
return {
nodeType: 'function',
type: node.type,
props: { ...node.pendingProps },
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels fundamentally wrong, but it works? I'm not sure how React implements the fiber nodes for forward refs under the hood but it's possible memoizedProps is never equal to pendingProps...

Copy link
Copy Markdown
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please never ever create duplicate PRs; this clutters the repo forever.

This will now have to stay open until #1592 is merged, and I'll have to manually and painfully keep it in sync, otherwise I'll have to forever look at the redundant PR ref on my git log.

case ContextProviderType: // 13
case ContextConsumerType: // 12
return childrenToTree(node.child);
case ForwardRefType: {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this logic to the 16.3 adapter in addition to the 16 adapter because React.forwardRef was added in 16.3.

@ljharb
Copy link
Copy Markdown
Member

ljharb commented Aug 8, 2018

Duplicate of #1592.

@suchipi
Copy link
Copy Markdown
Contributor Author

suchipi commented Aug 8, 2018

@ljharb should this PR be closed?

Alternatively, I can force-push the branch to contain nothing?

I don't understand what workflow you're optimizing around.

@ljharb
Copy link
Copy Markdown
Member

ljharb commented Aug 9, 2018

@suchipi no worries, i know it's probably not a common one.

Continue pushing to this one; I'll rebase and force push it so that both PRs match (once tests are passing here)

@ljharb
Copy link
Copy Markdown
Member

ljharb commented Aug 16, 2018

@suchipi thank you; these additional commits both add the missing test from #1592 and fix the other failures. It's very appreciated.

@ljharb ljharb merged commit 965b785 into enzymejs:master Aug 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

forwardRef: Enzyme Internal Error: unknown node with tag 14

3 participants