Named asset import for SVG files by iansu · Pull Request #3907 · react/create-react-app

iansu · 2018-01-23T18:30:28Z

Adds a babel plugin that transforms named SVG imports into components via a webpack loader. Usage: import { default as Logo } from './logo.svg';

The loader currently being used svg-react-loader doesn't provide a named export which is why we have to import default above. This can potentially be fixed in the new babel plugin, by making a PR to svg-react-loader or by using a different loader.

Closes#3856

…der.

gaearon

Can we also add an end to end test case that would have failed if we regressed on SVG URLs again?

We probably have an existing one that just doesn’t check the computed style. Maybe we could change it to use getComputedStyle and verify the string contains a proper URL. (You’d need to first check that this is implemented in jsdom.)

gaearon · 2018-01-23T18:33:52Z

+ "index.js"
+ ],
+ "dependencies": {
+ "@babel/core": "7.0.0-beta.38"


Why? Maybe this should be a peer dependency?

I originally wrote this plugin outside create-react-app so I needed this. I can change it to a peer dependency.

iansu · 2018-01-23T18:38:37Z

I haven't added any tests yet. I wanted to start getting feedback as soon as possible. An e2e test for the CSS use case is definitely on this list. I'd also like to add some tests to the babel plugin itself.

gaearon · 2018-01-23T18:37:38Z

 regenerator: true,
 },
 ],
+ [


This should probably have an opt-out flag in options. Just like the flow flag we now have.

gaearon · 2018-01-23T18:39:27Z

+ {
+ loaderMap: {
+ svg: filename => `-!svg-react-loader!${filename}`,
+ },


Actually, maybe this should be in webpack Babel config and not in our preset? Since it’s directly related to webpack specifically. This also would make a peer dependency on the loader unnecessary (it currently would be).

Yeah, either works. The only nice thing about keeping it here is that we don't have to duplicate it in the dev and prod webpack configs.

We can solve it in other ways (e.g. by actually starting to modularizing configs a little bit). Doesn't have to be solved here.

gaearon · 2018-01-25T12:27:52Z

What's the pending work here?

iansu · 2018-01-25T17:14:59Z

Remaining items:

Move plugin config to webpack config files
Fix e2e tests for including SVG component
Add e2e tests for including an SVG via CSS
Figure out how to change the name of the named import from default to ReactComponent. We could either submit a PR to svg-react-loader to add a named import or we could handle this in the plugin with some kind of setting to map the names.

gaearon · 2018-01-25T17:16:40Z

What was the reason we didn't use svgr loader?

iansu · 2018-01-25T17:19:17Z

I tried svgr, react-svg-loader and svg-react-loader. The only one I could get to work in this configuration was svg-react-loader. I believe this is because the other two expect you to run their output through babel-loader and I couldn't get that to work with webpack's loader query string syntax.

iansu · 2018-01-25T17:31:33Z

@gaearon What are your thoughts on how to handle point number 4?

gaearon · 2018-01-25T17:43:21Z

To be honest svgr loader seems much more actively maintained. I'd prefer if we work with them to figure out how to handle this.

iansu · 2018-01-25T17:50:13Z

I agree, that was a concern I had with svg-react-loader as well. I meant to include adding this functionality to svgr as an option. 😀

Ping @neoziro.

gregberge · 2018-01-26T20:22:52Z

Why not passing babel-loader in query string? I could add an option to do it but I think it is not a good approach.

iansu · 2018-01-26T20:43:58Z

I tried passing the loader in the query string but that means we also have to pass it a bunch of config too and I wasn't able to get it to work.

gregberge · 2018-01-27T14:14:11Z

You can't avoid running Babel on these files, I can't decide for you the config to apply and what is the browser targeted. react-svg-loader applies an arbitrary Babel config and I think it is not a good approach.

Personally I think that the babel + Webpack query string is too tricky. As I said in #3856, it could be solved using Webpack issuer. And if we want to support multiple loader for a same resource, it should be a Webpack feature or integrated in loader (like I did for svgr + url-loader).

gaearon · 2018-01-27T15:15:52Z

I can't decide for you the config to apply and what is the browser targeted

Is there a large difference in possible output though? As far as I can tell just emitting ES5 with createElement calls is always safe. And I don't see any benefit in targeting ES6 specifically even if the browser supports it.

gregberge · 2018-01-27T15:20:42Z

As far as I can tell just emitting ES5 with createElement calls is always safe.

Yes it is. But what about modules? Should I use import or require?

gaearon · 2018-01-27T15:22:57Z

That seems like the only point of contention so it could be toggled with a flag ?modules, similar to how Babel presets do it.

gaearon · 2018-01-27T15:23:51Z

Always emitting ESM also sounds good to me.

gregberge · 2018-01-27T15:26:54Z

@gaearon OK if you need it, I can implement it. It would also reduce boilerplate https://github.com/smooth-code/svgr#webpack-usage 😉.

gregberge · 2018-01-27T15:34:01Z

I created an issue on SVGR. I will do it tomorrow if nobody has taken it. gregberge/svgr#45

@gaearon @iansu feel free to precise the issue on SVGR, the first goal is to be compatible with your work.

gaearon · 2018-01-27T15:45:58Z

Here's another thing probably worth doing. React has certain optimizations when the element is referentially equal.

I see you allow overriding svg props (so it's dynamic), but everything inside is static.
Ideally the output would be like:

import{createElement}from'react';varcontent=createElement(/* everything inside svg */);functionMyIcon(props){returncreateElement('svg',props,content);}

The benefit is that if content is always the same, this makes it easy for React to bail out.

gregberge · 2018-01-31T22:23:42Z

This is done and released in svgr v1.8, I included babel-transform-react-constant-elements as you suggested @gaearon.

It should work out of the box without babel-loader 👍

iansu · 2018-01-31T23:30:53Z

Awesome! I'll update this PR to use the new version.

gaearon · 2018-02-01T00:05:52Z

One thing to keep in mind is that constant-elements transform has been a source of bugs in complex codebases (you can search Babel issues for matches, you'll find dozens of them).

It probably is fine when the output is simple and fully under our control (i.e. just SVG nodes). Just something to be aware of in case the generated code is more sophisticated.

It usually breaks down in combination with other complex transforms.

iansu · 2018-02-01T01:21:24Z

The new svgr loader works great. I still have to add/fix tests.

gregberge · 2018-02-01T07:20:22Z

One thing to keep in mind is that constant-elements transform has been a source of bugs in complex codebases (you can search Babel issues for matches, you'll find dozens of them).

I know, using it on a complete codebase is kind of tricky (and even trickier if you do not use it from start). But as you said, I think the plugin can do the job for SVG.

I can't apply optimization in a dummy way because I have several options in SVGR that make SVG depending on props.

gaearon · 2018-02-01T10:39:40Z

Makes sense. Yeah should be fine.

gaearon · 2018-02-01T19:59:24Z

 },
 ],
+ [
+ require('babel-plugin-named-asset-import'),


This shouldn't be enabled in the test environment.

I was just about to ask for suggestions on how to fix these tests. 😀
I was trying to find a way to use Jest's moduleNameMapper to do something with the webpack loader strings but I will try just disabling the plugin in the test environment.

iansu · 2018-02-01T21:18:13Z

+ it('svg component', async () => {
+ const doc = await initDOM('svg-component');
+
+ expect(doc.getElementById('feature-svg-component').textContent).to.equal(


I realize this test is not terribly useful but I'm not sure what I can check for here. I can try and improve it or just remove it entirely.

Maybe we can do getComputedStyle here?

This is for the SVG component not SVG in CSS so I don't think it will have any style?

Oh okay. Then can’t we test that innerHTML contains some SVG tags?

We're not getting a real SVG here because we mock it in fileTransform.js like so: ReactComponent: () => ${assetFilename}. That's why I initially checked the textContent for logo.svg but it's empty. I think the parent element would contain the filename but I'm not sure.

If that works let’s test for that

I have no idea if that works (or how to do it) but I'll give it a try. 😀

gaearon · 2018-02-01T21:23:57Z

 "react-dev-utils": "^5.0.0",
 "style-loader": "0.19.1",
- "svgr": "1.6.0",
+ "svgr": "^1.8.1",


Please keep the version pinned

gaearon · 2018-02-01T21:25:37Z

 },
 ],
+ !isEnvTest && [
+ require('babel-plugin-named-asset-import'),


What do you think about moving this to webpack configs instead of keeping it in the preset? That would make more sense to me.

I know we talked about that before and I tried it but I couldn't get it to work. For some reason when I move this to the webpack config it strips out functions. In this case the svg key would be present in the loaderMap object but it would just be an empty object. So the ReactComponent key would be removed presumably because its value is a function.

Can you push a commit that tries to implement it, even if it doesn’t work? I think we need to figure out what breaks there. Fine to do as another PR against your existing PR if you prefer so

Actually try rebasing. We don’t write babelrc anymore (afaik) since monorepos landed. So it doesn’t get serialized on eject and thus it may be solved now

I merged the next branch (with the monorepo stuff) into this branch a couple of commits ago. That should have the same effect right? You're saying I should try moving this to the webpack config again and see if it works now?

gaearon

Feel free to merge if you figure out how to move this out of the Babel preset

iansu · 2018-02-01T21:42:00Z

Okay, I'll work on it tonight and see if I can get it working. If not I'll push what I've got and we can try and figure it out.

iansu · 2018-02-02T05:15:18Z

I figured out the issue with passing a function to plugin options in the webpack config. It's thread-loader. With thread-loader enabled the passed function is stripped from the options object. With thread-loader removed everything works as expected. 😞

I'm not really sure how to proceed. I don't think we want to remove thread-loader so maybe we have to keep this config in the babel preset? I tried to make the plugin non-webpack specific but it's hard to get around that in the plugin config. Is there anything else we can do to make it more generic? Any other ideas?

gaearon · 2018-02-02T11:31:38Z

Let’s make the config serializable then.

 loaderMap: {svg: {ReactComponent: 'svgr/webpack![path]',},}

Then do a string replacement for [path].

gaearon · 2018-02-02T19:43:22Z

 babelrc: false,
 // @remove-on-eject-end
 presets: [require.resolve('babel-preset-react-app')],
+ plugins: [


Follow up: this will need an explanatory comment for what it does

gaearon · 2018-02-02T19:43:34Z

 babelrc: false,
 // @remove-on-eject-end
 presets: [require.resolve('babel-preset-react-app')],
+ plugins: [


See comment below

gaearon · 2018-02-02T19:44:34Z

+ const doc = await initDOM('svg-component');
+
+ expect(doc.getElementById('feature-svg-component').textContent).to.equal(
+ ''


I still want a better test here

* Add named asset import for svg files via babel plugin and webpack loader. * Fix failing e2e test * Switched to svgr loader * Updated SVG component test * Disable named asset import plugin in test environment * Added tests for including SVG in CSS * Update tests * Moved babel plugin config into webpack config

Add named asset import for svg files via babel plugin and webpack loa…
45d6324
…der.

facebook-github-bot added the CLA Signed label Jan 23, 2018

iansu added this to the 2.0.0 milestone Jan 23, 2018

gaearon reviewed Jan 23, 2018
View reviewed changes

Fix failing e2e test
3a1e13f

gregberge mentioned this pull request Jan 27, 2018
Include Babel transformation in Webpack loader gregberge/svgr#45
Closed

Switched to svgr loader
3d1de03

Updated SVG component test
f172ee3

gaearon reviewed Feb 1, 2018
View reviewed changes

iansu added 4 commits February 1, 2018 13:05

Disable named asset import plugin in test environment
8cdc51a

Added tests for including SVG in CSS
1e2d8a5

Update tests
d27b488

Merge next into branch
68439ac

iansu commented Feb 1, 2018
View reviewed changes

gaearon reviewed Feb 1, 2018
View reviewed changes

gaearon approved these changes Feb 1, 2018
View reviewed changes

Moved babel plugin config into webpack config
f047ac0

gaearon reviewed Feb 2, 2018
View reviewed changes

iansu merged commit d0e1731 into react:nextFeb 2, 2018

FWeinb mentioned this pull request Mar 27, 2018
Add repository in package.json FWeinb/babel-plugin-named-asset-import#1
Closed

just-boris mentioned this pull request Jun 18, 2018
Named asset import plugin doesn't work with export-as syntax #4640
Closed

This was referenced Jul 20, 2018
Override SVGO config using .svgrrc file gregberge/svgr#143
Closed
Upgrade webpack config to use @svgr/webpack loader #4798
Closed

lockBot locked and limited conversation to collaborators Jan 20, 2019

iansu deleted the named-asset-import branch October 18, 2019 05:54

Uh oh!

Uh oh!

Conversation

iansu commented Jan 23, 2018

Uh oh!

gaearon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iansu commented Jan 23, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaearon commented Jan 25, 2018

Uh oh!

iansu commented Jan 25, 2018

Uh oh!

gaearon commented Jan 25, 2018

Uh oh!

iansu commented Jan 25, 2018

Uh oh!

iansu commented Jan 25, 2018

Uh oh!

gaearon commented Jan 25, 2018

Uh oh!

iansu commented Jan 25, 2018

Uh oh!

gregberge commented Jan 26, 2018

Uh oh!

iansu commented Jan 26, 2018

Uh oh!

gregberge commented Jan 27, 2018

Uh oh!

gaearon commented Jan 27, 2018

Uh oh!

gregberge commented Jan 27, 2018

Uh oh!

gaearon commented Jan 27, 2018

Uh oh!

gaearon commented Jan 27, 2018

Uh oh!

gregberge commented Jan 27, 2018

Uh oh!

gregberge commented Jan 27, 2018

Uh oh!

gaearon commented Jan 27, 2018

Uh oh!

gregberge commented Jan 31, 2018

Uh oh!

iansu commented Jan 31, 2018

Uh oh!

gaearon commented Feb 1, 2018

Uh oh!

iansu commented Feb 1, 2018

Uh oh!

gregberge commented Feb 1, 2018

Uh oh!

gaearon commented Feb 1, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!