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

Forward ref #1738

Merged
merged 6 commits into from
Aug 16, 2018
Merged

Forward ref #1738

merged 6 commits into from
Aug 16, 2018

Conversation

suchipi
Copy link
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
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
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.

@@ -133,6 +134,17 @@ function toTree(vnode) {
case ContextProviderType: // 13
case ContextConsumerType: // 12
return childrenToTree(node.child);
case ForwardRefType: {
Copy link
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
Member

ljharb commented Aug 8, 2018

Duplicate of #1592.

@suchipi
Copy link
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
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
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
@ljharb ljharb added this to Refs in React 16 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
React 16
  
Refs
Development

Successfully merging this pull request may close these issues.

forwardRef: Enzyme Internal Error: unknown node with tag 14
3 participants