-
Notifications
You must be signed in to change notification settings - Fork 94
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
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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
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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently, when transforming an SVG file that has attributes on the
top-level
<svg>
element, the transformed component ends up lookingsomething like this:
this adds the overhead of _extends. I think it might be a better to
instead move these values into
defaultProps
on the generatedcomponent.
Here's the diff of the result:
Closes #7