Add forward ref to React SVG Component

[GasimGasimzada]

This pull request adds "ref" option to SVGR loader; so that, the root svg component ref can be accessed. The way it works is that SVGR creates a forward ref and passed the ref to svg component using svgRef prop.

Example:

import React, { Component } from 'react';
import { ReactComponent as Logo } from './logo.svg'; // CRA default logo file

class App extends Component {
    myRef = React.createRef();

    componentDidMount() {
        console.log(this.myRef);
    }

    render() {
        return (<Logo svgRef={this.myRef} />);
     }
}

This example will print the svg element.

Testing

I have added unit tests to webpack/SVGComponent.js; however, I was not able to perform the test. yarn e2e:docker -- --test-suite kitchensin kept giving me permission denied error inside the container.

[facebook-github-bot]

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

[iansu]

This looks good. I think we just need to get the tests passing.

[GasimGasimzada]

Okay. Then, I’ll try to figure out how to perform the tests and fix if there are any problems.

[GasimGasimzada]

There was a need to update Jest file transform for SVG to include ref via svgRef prop. Also some additional fixes were necessary. I still was not able to get yarn e2e:docker working to test it in my own environment. The error says permission denied on some integrity related package (I think it was node integrity).

[gaearon]

We shouldn't need a separate prop for this. Instead there should be a way to tell svgr to use React.forwardRef which does the same thing directly for the ref attribute.

[GasimGasimzada]

Adding “ref” option to SVGR webpack config does add forwardRef to the SVG component. However, the prop that is being used to pass ref is svgRef when SVGR component is imported. That’s not something that I assigned. I have looked into SVGR docs to change the name to ref but I it is hard coded in their codebase.

[gaearon]

I don't quite understand what it means. The sole purpose of forwardRef is to allow you to use ref attribute. It doesn't let you do anything else.

[iansu]

I think ref points to the React component that svgr returns, which wraps an svg element that is pointed to by svgRef. So just using ref would give you the wrapper component, not the svg itself.

[GasimGasimzada]

To be completely honest, I dont quiet understand it either. I tried to use ref in CRA and kept getting error that stateless components cannot have refs; they should be assigned to elements. This is after forward ref is enabled and is the default export when SVGs are being imported using SVGR (I printed the imported component and it does have ref). However, using svgRef works fine.

I might be missing something here and I’ll investigate to see what the issue might be.

[gaearon]

Where does svgr use forwardRef? Maybe their usage is incorrect.

[GasimGasimzada]

This is the template directory for SVGR Transform: https://github.com/smooth-code/svgr/tree/master/packages/core/src/templates

util.js is where forwardRef is implemented + the reactDOMTemplate.js for the main template.

[gaearon]

Apparently it was added specifically for a library called react-native-svg which is where the svgRef convention comes from. :-/ gregberge/svgr@4bdd989

@neoziro Can we make the solution generic so it works with ReactDOM? Ideally it's react-native-svg that should be fixed to use forwardRef instead of having its own "svgRef" prop.

[gregberge]

@gaearon the latest version of SVGR supports forwardRef but it is not supported by the version used in create-react-app. It is nothing relative with "react-native-svg", it is just a feature to enable "ref", I don't do anything different for native.

You can try it here in https://svgr.now.sh/.

[gregberge]

@gaearon if you want to upgrade SVGR, you should probably wait for the next version. I will change a lot of things:

• Use rehype instead of JSDOM
• Replace h2x by Babel
• Do not include "prettier" and "svgo" by default

So it will result by a huge performance improvement and a smaller package size.

I plan to release it in one or two weeks.

[GasimGasimzada]

I completely forgot about checking what version the ref was added to SVGR and got confused why ref was not working :/ Upgrading locally to v3.1.0 worked fine with. I modified the test files to use refs instead of svgRef and added forward ref to the SVG transform for Jest. However, the packages are not upgraded; so, the tests are failing (locally everything works as expected).

[iansu]

@neoziro This is good to know. I was considering updating CRA to use SVGR 3 but now I'll wait for the new version to be released. Thanks!

[stale]

This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed in 5 days if no further activity occurs.

[gregberge]

@iansu any update?

[iansu]

There's a PR to upgrade to SVGR 4: #5816. I think it makes sense to merge that first and then update and merge this branch.

[GasimGasimzada]

@iansu Any update on this?

[iansu]

@GasimGasimzada Now that SVGR has been upgraded you should be able to update this branch. A few things have changed, for example our webpack config is now in a single file instead of separate dev and prod files. Once your branch is updated and everything is working we should be able to merge this.

[GasimGasimzada]

@iansu Great! I will make my changes by tomorrow then :)

[GasimGasimzada]

I have updated SVGR configuration in the new webpack config.

[iansu]

Thanks!