babel-plugin-inline-react-svg icon indicating copy to clipboard operation
babel-plugin-inline-react-svg copied to clipboard

Provide option for spreading props rather than static assignment

Open atkinchris opened this issue 5 years ago • 13 comments

This provides an option (spreadDefaultProps) to spread the extra props from the top level svg element onto the props object, rather than statically assigning them as defaultProps. This gives users an optional trade off for _spread performance (as raised in the PR that originally setup defaultProps) against being able to tree shake the generated components (which is prevented if they have static assignments).

The README has been updated, and a sanity test scenario added.

This also corrects some bugs which arise when opts.SVG_DEFAULT_PROPS_CODE is not assigned.

The first is that Babel will throw an error when a substitution key is provided but not used, even if it's value is undefined. This happens when opts.SVG_DEFAULT_PROPS_CODE is not assigned, and therefore the SVG_NAME.defaultProps = SVG_DEFAULT_PROPS_CODE template string is never included - however, the SVG_DEFAULT_PROPS_CODE is still passed.

The second is using replaceWith versus replaceWithMultiple when there are multiple nodes to be replaced. This is currently predicated on opts.SVG_DEFAULT_PROPS_CODE - which is not accurate. It should be predicated on if there are multiple nodes (svgReplacement.length > 1).

atkinchris avatar Aug 08 '20 10:08 atkinchris

defaultProps is being deprecated on function components (https://github.com/facebook/react/pull/25699), therefore it would be good to land something like this.

(In fact, I'm getting the warning using Next.js today: Support for defaultProps will be removed from function components in a future major release. Use JavaScript default parameters instead.)

jpveooys avatar Jun 22 '23 15:06 jpveooys

That's the plan, but since it's a terrible idea, I think it's worth waiting until it ships, and the React team responds to the inevitable fallout, before treating that as gospel.

ljharb avatar Jun 23 '23 03:06 ljharb

hey @atkinchris was this issue resolved or did you find a workaround? i am currently experiencing the same error all over my next app due to deprecated defaultProps being used when transforming svgs

KodinManiac avatar Nov 10 '23 11:11 KodinManiac

If it's just next.js issuing that warning, I'd suggest filing an issue with them to remove it, since it's clearly premature.

ljharb avatar Nov 10 '23 17:11 ljharb

With the release of React 18.3, the defaultProps warning is now in a stable release of React and will affect many more developers.

@ljharb, would you be open to the spreadDefaultProps configuration option now, so developers can decide between the two approaches themselves? 🙏

Out of curiosity, what kind of fallout are you anticipating? What negative side effects should developers be concerned about? The only two discussions on defaultProps I could find on the original RFC seem to have been answered.

connor-baer avatar Apr 26 '24 19:04 connor-baer

There’s a lot of benefits from propTypes and defaultProps, both in runtime effect and tooling introspection/composition, but since react has continued with this plan anyways, we might as well add the option.

ljharb avatar Apr 27 '24 00:04 ljharb

Any plans on releasing this?

einarq avatar Apr 30 '24 07:04 einarq