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 React forwardRef in pretty-format #6076

Closed
wants to merge 5 commits into from

Conversation

jkimbo
Copy link
Contributor

@jkimbo jkimbo commented Apr 26, 2018

Summary

#6069 tried to add support for React.forwardRef formatting but it doesn't seem to work for me. I've got it working by adding a new plugin ReactForwardRef that handles forwardRef "elements". Had to create a separate plugin since the forwardRef spec doesn't conform to normal React elements: https://github.com/facebook/react/blob/920f30ef7732e87045ae6652c464b58267991b8f/packages/react/src/forwardRef.js

Test plan

Run tests and make sure they pass

@codecov-io
Copy link

codecov-io commented Apr 26, 2018

Codecov Report

Merging #6076 into master will increase coverage by 0.02%.
The diff coverage is 88.88%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6076      +/-   ##
==========================================
+ Coverage   64.13%   64.16%   +0.02%     
==========================================
  Files         217      218       +1     
  Lines        8335     8344       +9     
  Branches        4        3       -1     
==========================================
+ Hits         5346     5354       +8     
- Misses       2988     2989       +1     
  Partials        1        1
Impacted Files Coverage Δ
packages/pretty-format/src/index.js 96.37% <ø> (ø) ⬆️
...ges/pretty-format/src/plugins/react_forward_ref.js 88.88% <88.88%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c608b09...b0c5d3a. Read the comment docs.

@cpojer cpojer requested a review from mjesun April 26, 2018 19:11
@mjesun
Copy link
Contributor

mjesun commented Apr 28, 2018

cc @camspiers can you double-check? Thanks!

depth: number,
refs: Refs,
printer: Printer,
): string => printElementAsLeaf(getType(element), config);
Copy link
Member

Choose a reason for hiding this comment

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

can we make this part of the normal ReactElementPlugin instead of creating a new one?

Copy link
Member

Choose a reason for hiding this comment

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

Hah, I should read the OP more closely

Had to create a separate plugin since the forwardRef spec doesn't conform to normal React elements

@jkimbo
Copy link
Contributor Author

jkimbo commented Apr 29, 2018

Ok so I've looked into this a bit more and I don't think this is the right way to solve it. Think I have a better solution that I'll write up today so I'm going to close this PR in the mean time.

@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants