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

Named asset import for SVG files #3907

Merged
merged 9 commits into from Feb 2, 2018

Conversation

Projects
None yet
4 participants
@iansu
Copy link
Collaborator

iansu commented Jan 23, 2018

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

@iansu iansu added this to the 2.0.0 milestone Jan 23, 2018

@gaearon
Copy link
Member

gaearon left a comment

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.)

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

This comment has been minimized.

@gaearon

gaearon Jan 23, 2018

Member

Why? Maybe this should be a peer dependency?

This comment has been minimized.

@iansu

iansu Jan 23, 2018

Collaborator

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

@iansu

This comment has been minimized.

Copy link
Collaborator

iansu commented Jan 23, 2018

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.

@@ -115,6 +115,14 @@ module.exports = function(api, opts) {
regenerator: true,
},
],
[

This comment has been minimized.

@gaearon

gaearon Jan 23, 2018

Member

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

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

This comment has been minimized.

@gaearon

gaearon Jan 23, 2018

Member

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).

This comment has been minimized.

@iansu

iansu Jan 23, 2018

Collaborator

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.

This comment has been minimized.

@gaearon

gaearon Jan 23, 2018

Member

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

This comment has been minimized.

Copy link
Member

gaearon commented Jan 25, 2018

What's the pending work here?

@iansu

This comment has been minimized.

Copy link
Collaborator

iansu commented Jan 25, 2018

Remaining items:

  1. Move plugin config to webpack config files
  2. Fix e2e tests for including SVG component
  3. Add e2e tests for including an SVG via CSS
  4. 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

This comment has been minimized.

Copy link
Member

gaearon commented Jan 25, 2018

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

@iansu

This comment has been minimized.

Copy link
Collaborator

iansu commented Jan 25, 2018

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

This comment has been minimized.

Copy link
Collaborator

iansu commented Jan 25, 2018

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

@gaearon

This comment has been minimized.

Copy link
Member

gaearon commented Jan 25, 2018

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

This comment has been minimized.

Copy link
Collaborator

iansu commented Jan 25, 2018

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.

@neoziro

This comment has been minimized.

Copy link

neoziro commented Jan 26, 2018

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

This comment has been minimized.

Copy link
Collaborator

iansu commented Jan 26, 2018

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.

@neoziro

This comment has been minimized.

Copy link

neoziro commented Jan 27, 2018

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

This comment has been minimized.

Copy link
Member

gaearon commented Jan 27, 2018

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.

@neoziro

This comment has been minimized.

Copy link

neoziro commented Jan 27, 2018

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

This comment has been minimized.

Copy link
Member

gaearon commented Jan 27, 2018

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

This comment has been minimized.

Copy link
Member

gaearon commented Jan 27, 2018

Always emitting ESM also sounds good to me.

@neoziro

This comment has been minimized.

Copy link

neoziro commented Jan 27, 2018

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

@neoziro

This comment has been minimized.

Copy link

neoziro commented Jan 27, 2018

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

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

@gaearon

This comment has been minimized.

Copy link
Member

gaearon commented Jan 27, 2018

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';

var content = createElement(/* everything inside svg */);

function MyIcon(props) {
  return createElement('svg', props, content);
}

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

@neoziro

This comment has been minimized.

Copy link

neoziro commented Jan 31, 2018

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

This comment has been minimized.

Copy link
Collaborator

iansu commented Jan 31, 2018

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

@gaearon

This comment has been minimized.

Copy link
Member

gaearon commented Feb 1, 2018

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

This comment has been minimized.

Copy link
Collaborator

iansu commented Feb 1, 2018

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

@neoziro

This comment has been minimized.

Copy link

neoziro commented Feb 1, 2018

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

This comment has been minimized.

Copy link
Member

gaearon commented Feb 1, 2018

Makes sense. Yeah should be fine.

@@ -115,6 +115,16 @@ module.exports = function(api, opts) {
regenerator: true,
},
],
[
require('babel-plugin-named-asset-import'),

This comment has been minimized.

@gaearon

gaearon Feb 1, 2018

Member

This shouldn't be enabled in the test environment.

This comment has been minimized.

@iansu

iansu Feb 1, 2018

Collaborator

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 added some commits Feb 1, 2018

it('svg component', async () => {
const doc = await initDOM('svg-component');

expect(doc.getElementById('feature-svg-component').textContent).to.equal(

This comment has been minimized.

@iansu

iansu Feb 1, 2018

Collaborator

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.

This comment has been minimized.

@gaearon

gaearon Feb 1, 2018

Member

Maybe we can do getComputedStyle here?

This comment has been minimized.

@iansu

iansu Feb 1, 2018

Collaborator

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

This comment has been minimized.

@gaearon

gaearon Feb 1, 2018

Member

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

This comment has been minimized.

@iansu

iansu Feb 1, 2018

Collaborator

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.

This comment has been minimized.

@gaearon

gaearon Feb 1, 2018

Member

If that works let’s test for that

This comment has been minimized.

@iansu

iansu Feb 1, 2018

Collaborator

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

@@ -56,7 +56,7 @@
"raf": "3.4.0",
"react-dev-utils": "^5.0.0",
"style-loader": "0.19.1",
"svgr": "1.6.0",
"svgr": "^1.8.1",

This comment has been minimized.

@gaearon

gaearon Feb 1, 2018

Member

Please keep the version pinned

This comment has been minimized.

@iansu

iansu Feb 1, 2018

Collaborator

Will do.

@@ -115,6 +115,16 @@ module.exports = function(api, opts) {
regenerator: true,
},
],
!isEnvTest && [
require('babel-plugin-named-asset-import'),

This comment has been minimized.

@gaearon

gaearon Feb 1, 2018

Member

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

This comment has been minimized.

@iansu

iansu Feb 1, 2018

Collaborator

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.

This comment has been minimized.

@gaearon

gaearon Feb 1, 2018

Member

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

This comment has been minimized.

@gaearon

gaearon Feb 1, 2018

Member

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

This comment has been minimized.

@iansu

iansu Feb 1, 2018

Collaborator

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?

This comment has been minimized.

@gaearon
@gaearon

gaearon approved these changes Feb 1, 2018

Copy link
Member

gaearon left a comment

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

@iansu

This comment has been minimized.

Copy link
Collaborator

iansu commented Feb 1, 2018

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

This comment has been minimized.

Copy link
Collaborator

iansu commented Feb 2, 2018

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

This comment has been minimized.

Copy link
Member

gaearon commented Feb 2, 2018

Let’s make the config serializable then.

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

Then do a string replacement for [path].

@@ -200,6 +200,18 @@ module.exports = {
babelrc: false,
// @remove-on-eject-end
presets: [require.resolve('babel-preset-react-app')],
plugins: [

This comment has been minimized.

@gaearon

gaearon Feb 2, 2018

Member

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

@@ -192,6 +192,18 @@ module.exports = {
babelrc: false,
// @remove-on-eject-end
presets: [require.resolve('babel-preset-react-app')],
plugins: [

This comment has been minimized.

@gaearon

gaearon Feb 2, 2018

Member

See comment below

const doc = await initDOM('svg-component');

expect(doc.getElementById('feature-svg-component').textContent).to.equal(
''

This comment has been minimized.

@gaearon

gaearon Feb 2, 2018

Member

I still want a better test here

@iansu iansu merged commit d0e1731 into facebook:next Feb 2, 2018

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

akstuhl added a commit to akstuhl/create-react-app that referenced this pull request Mar 15, 2018

Named asset import for SVG files (facebook#3907)
* 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

zhDmitry pushed a commit to zhDmitry/create-react-app that referenced this pull request Sep 30, 2018

Named asset import for SVG files (facebook#3907)
* 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

@lock lock bot locked and limited conversation to collaborators Jan 20, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.