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

fix: wrap svg component directly with memo/forwardRef (#440) #441

Merged
merged 4 commits into from
Apr 27, 2020

Conversation

katywings
Copy link
Contributor

@katywings katywings commented Apr 26, 2020

Summary

Already described in: #440 - there is also a demo :D

Root cause for the typescript error

By wrapping the svg component in a new component at

result += `const ${nextExportName} = React.forwardRef((props, ref${refType}) => <${exportName} svgRef={ref} {...props} />)\n\n`
the generic types of React.forwardRef loose the prop types from the svg component.

Solution

This fix directly wraps the component in React.forwardRef without something in between. Not wrapping the component in an empty proxy component also has a slight performance advantage. To accomplish the direct wrap with React.forwardRef I also had to change the order with React.memo: React.forwardRef has to be the first wrapper around the svg component because React.memo itself doesnt forward the ref further - the difference can be seen here: https://github.com/gregberge/svgr/pull/441/files#diff-fd642229e3068089af3cc1b24827673aR147

Output changes

To summarize what this changes in the svg component (taken from "plugin typescript with "ref" and "expandProps" option expands props 1" before/after snapshots):

OLD:

interface SVGRProps {
  svgRef?: React.Ref<SVGSVGElement>
}

function SvgComponent({
  svgRef,
  ...props
}: React.SVGProps<SVGSVGElement> & SVGRProps) {
  return <svg><g /></svg>;
}

const ForwardRef = React.forwardRef((props, ref: React.Ref<SVGSVGElement>) => <SvgComponent svgRef={ref} {...props} />);
export default ForwardRef;

NEW:

function SvgComponent(props: React.SVGProps<SVGSVGElement>, svgRef?: React.Ref<SVGSVGElement>) {
  return <svg><g /></svg>;
}

const ForwardRef = React.forwardRef(SvgComponent);
export default ForwardRef;

Of course this is only one of many option combinations that had slight changes as seen in the test commits, but it gives a good overview.

Test plan

I tested it with yarn test and also tried it out with several builds using feather-icons.

@vercel
Copy link

vercel bot commented Apr 26, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/gregberge/svgr/cb92iyfna
✅ Preview: https://svgr-git-fork-katywings-bugfix-forward-ref-expand-props.gregberge.now.sh

@katywings
Copy link
Contributor Author

P.S: when I started with the fix I didn't think that I would have to make so many changes xD, but I just couldn't stop in the middle

@codecov
Copy link

codecov bot commented Apr 26, 2020

Codecov Report

Merging #441 into master will increase coverage by 0.07%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #441      +/-   ##
==========================================
+ Coverage   86.53%   86.60%   +0.07%     
==========================================
  Files          31       31              
  Lines         557      560       +3     
  Branches      153      155       +2     
==========================================
+ Hits          482      485       +3     
  Misses         60       60              
  Partials       15       15              
Impacted Files Coverage Δ
...s/babel-plugin-transform-svg-component/src/util.js 95.58% <100.00%> (+0.20%) ⬆️

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 f30bb85...84370ea. Read the comment docs.

@gregberge
Copy link
Owner

Hello @katywings, it looks great! It simplifies code and fixes a bug, thank you!

@gregberge gregberge merged commit a6de2da into gregberge:master Apr 27, 2020
@katywings
Copy link
Contributor Author

@gregberge De nada! Thank you for merging it so soon :)

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.

2 participants