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

Import SVGs as React components (#1388) #3718

Merged
merged 8 commits into from
Jan 17, 2018
Merged

Conversation

iansu
Copy link
Contributor

@iansu iansu commented Jan 8, 2018

Use svgr webpack loader to wrap SVGs in React components as a named export. SVG filenames are still available as the default export.

import logo from './logo.svg'; // gives you the filename
import { ReactComponent as Logo } from './logo.svg'; // gives you a React component <Logo />

Closes #1388

@gaearon
Copy link
Contributor

gaearon commented Jan 9, 2018

Nice, will check it out tomorrow!

@gnapse
Copy link
Contributor

gnapse commented Jan 15, 2018

Thanks! This would be very nice to have. Can't wait to have it merged, as I'd hate to eject just for this.

@gaearon gaearon changed the base branch from master to next January 15, 2018 17:36
@iansu
Copy link
Contributor Author

iansu commented Jan 16, 2018

@gaearon I see this is on the 2.0 roadmap in the "likely but needs work" list. Let me know what I can do to get this ready.

@iansu
Copy link
Contributor Author

iansu commented Jan 16, 2018

I think the reason for doing this automatically is simplicity. While it's true that you can just do this yourself using the svgr binary that's extra work. You need to know to install a global package, you need to remember to rebuild any svg files you've changed and you need to keep both copies in your repo.

Doing it via webpack does't require the user to do any of these things. Webpack will also only compile any files that have changed so, while this does add a bit of overhead to the initial build, subsequent builds won't be affected.

@gaearon
Copy link
Contributor

gaearon commented Jan 16, 2018

@iansu It's the closest category to "landed", I think the only thing it needs is a review. I'll try to test it in the coming days but would appreciate if more people did that. Unfortunately I don't have a super easy way to release test builds right now.

@iansu
Copy link
Contributor Author

iansu commented Jan 16, 2018

@gaearon Sounds good. This will probably also require some doc updates. Is that something that should happen as part of this PR or separately?

@gaearon
Copy link
Contributor

gaearon commented Jan 16, 2018

It's okay to it in this PR if you add a note (similar to existing ones) that it only works with 2.0.

@gaearon
Copy link
Contributor

gaearon commented Jan 16, 2018

It might be easier though to wait a little, and do a doc PR sprint before the releas.e

@iansu
Copy link
Contributor Author

iansu commented Jan 16, 2018

Sounds good. I'll hold off for now.

@gaearon gaearon added this to the 2.0.0 milestone Jan 17, 2018
@@ -235,6 +235,31 @@ module.exports = {
},
],
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we need a change in production config too?

babelrc: false,
presets: [require.resolve('babel-preset-react-app')],
// @remove-on-eject-end
// This is a feature of `babel-loader` for webpack (not Babel itself).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably doesn't make sense to repeat this comment.

@gaearon
Copy link
Contributor

gaearon commented Jan 17, 2018

We'll need a new end-to-end test in fixtures/kitchensink for this. See existing tests for image inclusion for inspiration.

Copy link
Contributor

@gaearon gaearon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Please fix the production config.
  • A component relying on this shouldn't fail the test. Notice how if you add this to App.js then yarn test fails. To fix this you probably want to muck with packages/react-scripts/config/jest/fileTransform.js and add a special case there. It's fine to shim it with just an empty <svg> or something like this. Don't forget to re-run tests with yarn test --no-cache to check your fix.
  • Add a fixture similar to existing SvgInclusion.

@@ -15,6 +15,10 @@ const path = require('path');

module.exports = {
process(src, filename) {
if (filename.match(/\.svg$/)) {
return `module.exports = {ReactComponent: () => ('')};`;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we don’t want to render anything can this just return null?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not quite accurate. There should be two exports: default and named one. Tests that expect this to render to the filename shouldn’t start failing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't exactly sure what to return there. Null would probably work. Also wasn't sure about the exports. How do you return a default and named export using module.exports?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe something like

module.exports = {
  __esModule: true,
  default: name,
  ReactComponent: () => name
}

return `module.exports = {
__esModule: true,
default: ${JSON.stringify(path.basename(filename))},
ReactComponent: () => null
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not return the filename here as well? Seems like that would be more useful in e.g. snapshots.

describe('svg component', () => {
it('renders without crashing', () => {
const div = document.createElement('div');
ReactDOM.render(<SvgComponent />, div);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And in this test you could verify the div.textContent matches the filename.

@kellyrmilligan
Copy link
Contributor

@iansu this looks awesome! one question, will this hide the ability to replace attribute values? most svg icon workflows i've seen you need to have currentColor as the fill or stroke value to be able to style them with css. I see that's possible on the binary, but what about this?

akstuhl pushed a commit to akstuhl/create-react-app that referenced this pull request Mar 15, 2018
* Import SVGs as React components (facebook#1388)

* Updated webpack production config and fixed tests

* Improved Jest SVG file transform

* Improved SVG tests

* Add a comment

* Update webpack.config.prod.js
yoiang referenced this pull request in Adorkable-forkable/create-react-app-typescript May 2, 2018
* Import SVGs as React components (#1388)

* Updated webpack production config and fixed tests

* Improved Jest SVG file transform

* Improved SVG tests

* Add a comment

* Update webpack.config.prod.js

# Conflicts:
#	packages/react-scripts/package.json
@ianschmitz ianschmitz mentioned this pull request Jul 30, 2018
11 tasks
zmitry pushed a commit to zmitry/create-react-app that referenced this pull request Sep 30, 2018
* Import SVGs as React components (facebook#1388)

* Updated webpack production config and fixed tests

* Improved Jest SVG file transform

* Improved SVG tests

* Add a comment

* Update webpack.config.prod.js
@lock lock bot locked and limited conversation to collaborators Jan 20, 2019
@iansu iansu deleted the svg-component branch October 18, 2019 05:54
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants