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

Use defaultProps instead of adding props to the svg #8

Merged
merged 1 commit into from
Sep 26, 2017

Conversation

lencioni
Copy link
Contributor

@lencioni lencioni commented Mar 20, 2017

Currently, when transforming an SVG file that has attributes on the
top-level <svg> element, the transformed component ends up looking
something like this:

_extends({
  xmlns: 'http://www.w3.org/2000/svg',
  viewBox: '0 0 1000 1000'
}, props),

this adds the overhead of _extends. I think it might be a better to
instead move these values into defaultProps on the generated
component.

Here's the diff of the result:

diff --git a/master.js b/default-props.js
index cb187ed..e62cca0 100644
--- a/master.js
+++ b/default-props.js
@@ -9,8 +9,6 @@ exports.MyClassIcon = undefined;
 
 var _createClass = function () { function defineProperties(target, props) { for (var i = 0; i < props.length; i++) { var descriptor = props[i]; descriptor.enumerable = descriptor.enumerable || false; descriptor.configurable = true; if ("value" in descriptor) descriptor.writable = true; Object.defineProperty(target, descriptor.key, descriptor); } } return function (Constructor, protoProps, staticProps) { if (protoProps) defineProperties(Constructor.prototype, protoProps); if (staticProps) defineProperties(Constructor, staticProps); return Constructor; }; }();
 
-var _extends = Object.assign || function (target) { for (var i = 1; i < arguments.length; i++) { var source = arguments[i]; for (var key in source) { if (Object.prototype.hasOwnProperty.call(source, key)) { target[key] = source[key]; } } } return target; };
-
 exports.MyFunctionIcon = MyFunctionIcon;
 
 var _react = require('react');
@@ -29,12 +27,7 @@ var MySvg = function () {
   function MySvg(props) {
     return _react2['default'].createElement(
       'svg',
-      _extends({
-        dataName: 'Livello 1',
-        id: 'Livello_1',
-        viewBox: '0 0 151.57 151.57',
-        xmlns: 'http://www.w3.org/2000/svg'
-      }, props),
+      props,
       _react2['default'].createElement('title', null),
       _react2['default'].createElement('circle', {
         cx: '1038.5',
@@ -78,6 +71,12 @@ var MySvg = function () {
   return MySvg;
 }();
 
+MySvg.defaultProps = {
+  dataName: 'Livello 1',
+  id: 'Livello_1',
+  viewBox: '0 0 151.57 151.57',
+  xmlns: 'http://www.w3.org/2000/svg'
+};
 function MyFunctionIcon() {
   return _react2['default'].createElement(MySvg, null);
 }

Closes #7

Currently, when transforming an SVG file that has attributes on the
top-level `<svg>` element, the transformed component ends up looking
something like this:

```js
_extends({
  xmlns: 'http://www.w3.org/2000/svg',
  viewBox: '0 0 1000 1000'
}, props),
```

this adds the overhead of _extends. I think it might be a better to
instead move these values into `defaultProps` on the generated
component.

Closes airbnb#7
@kesne kesne merged commit c03fd77 into airbnb:master Sep 26, 2017
atkinchris added a commit to atkinchris/babel-plugin-inline-react-svg that referenced this pull request Aug 8, 2020
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`: airbnb#8) 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`).
vieira added a commit to vieira/babel-plugin-inline-react-svg that referenced this pull request Aug 30, 2024
Drop defaultProps from the generated component as they are now
deprecated and will be removed in React 19.

They were added in airbnb#8 due to concerns about the overhead of `_extends`
although no benchmarks were provided and it is likely that difference
would be marginal where it matters.

Fixes: airbnb#25
vieira added a commit to vieira/babel-plugin-inline-react-svg that referenced this pull request Aug 30, 2024
Drop defaultProps from the generated component as they are now
deprecated and will be removed in React 19.

They were added in airbnb#8 due to concerns about the overhead of `_extends`
although no benchmarks were provided and it is likely that difference
would be marginal where it matters.

Fixes: airbnb#126
atkinchris added a commit to atkinchris/babel-plugin-inline-react-svg that referenced this pull request Sep 10, 2024
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`: airbnb#8) 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 added a commit to atkinchris/babel-plugin-inline-react-svg that referenced this pull request Sep 10, 2024
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`: airbnb#8) 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`).
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