"SVG as a component" doesn't quite work in 2.x

Question

"SVG as a component" doesn't quite work in 2.x

gaearon opened this issue 7 years ago · 65 comments

gaearon commented 7 years ago

We merged #3718 and released the first 2.x alpha, but quickly discovered a few issues:

Importing SVG from CSS is now broken, you get [Object object] in URLs.
While that can be fixed by adding a named export called toString() here (which would just return the URL), it looks like the resulting JS bundle contains the React component as soon as CSS imports it. It's my impression that webpack CSS pipeline still relies on CommonJS and therefore can't tree shake the component.

I'm not sure if there's any solution even possible here 😢 We probably need to revert the feature.

Just in case, pinging @neoziro @iansu for more ideas.

gaearon commented 7 years ago

Thanks!

Answer 1 · 2018-01-18T13:51:04.000Z

OK, I have a different idea that can maybe solve both this and help #3722 in the future.

My main concern with #3722 is the build performance: we don't want to spend time building something we don't use. But the loader doesn't know what needs to be built if it's not directly in the "request" (of which the named import isn't a part). And the same exact problem is what's causing this bug.

I thought: what if we gave webpack more information without the user knowing about it? We could have an extra Babel transform that takes

import logoUrl, { ReactComponent as Logo } from './logo.svg';

and turns it into

// generated code
import logoUrl from './logo.svg';
import Logo from 'svgr-loader!./logo.svg';

(or similar)

The important part is this happens behind the scenes so the user doesn’t need to know about loaders. It does couple our Babel preset and webpack a little bit but it could be an opt-in option so people who use it without webpack won’t get this.

This way we can both have an expressive way to import what you need without webpack syntax, and a way for the loaders to know what exactly to generate. Inside CSS we’d always get the filenames (since we wouldn‘t transform it with Babel).

I won’t have time to work on this but @iansu maybe you’d be interested to try?

Answer 2 · 2018-01-18T15:17:53.000Z

@michael-ciniawsky

Any chance you could help us understand the best path forward here? It seems like we could avoid a hacky solution if file-loader and css-loader worked with ESM. Then svgr-loader/webpack could provide two exports (one for URL and one for a component), and when SVG is imported through CSS, it would only get the URL. Is this something that should work in the future? Is it blocked on webpack 4? Are there any hanging PRs that we could help with to push this feature through?

Answer 3 · 2018-01-18T15:43:45.000Z

@gaearon Not sure if this is solvable with the current CSS Pipeline and CJS (it's overdue to update some parts there anyways (e.g css-loader)), but should be with ESM (see below). I'm working on next already it's just not finished/ approved yet (~'90%') 😛

CSS Loader (`css-loader@next` (⚠️ not released))

https://github.com/webpack-contrib/css-loader/blob/next/src/index.js#L126

CSS => ESM (Formatting via `css-loader`)

main.css

@import './file.css'

.selector {
   background-image: url('./file.png');
}

module (main.css)

// CSS Imports
import CSS__URL__0 from './file.png' // url('./file.png') (Assets)
import CSS__IMPORT__0 from './file.css' // @import

// CSS Exports
// e.g CSS Modules (Named Exports)
export const name = 'file__selector---hash';

// CSS
export default `${css}`; // {String} (no css.toString() && 'runtime' anymore (needs triage))

CSS Plugin (`extract-text-webpack-plugin@next` (⚠️ not released))

https://gist.github.com/michael-ciniawsky/4c4427a185a1c33f065ea3069d144164

Extracts all CSS 'Modules' (ESM) per Chunk (Sync && Async)

chunk.css

/* file.css (@import) */
...css

/* main.css */
.selector {
   background-image: url('./3624tf43zziv.png');
}

`file/url-loader`

I write this up as gists, I have a few ideas via meta to enable something like ESM Exports/ isAsset but this needs use case analysis and triage. Could you open issues in css/url/file-loader to keep track of this ? If this isn't to CRA specific (or we find generic 'webpack' pattens) it's better to solve it in the loaders directly whenever possible :)

Answer 4 · 2018-01-18T15:45:05.000Z

'Tree-shaking'/Dedupe works (so far), since the CSS is 💯 ESM then

Answer 5 · 2018-01-18T16:00:03.000Z

It's also possible to add custom imports/exports to the CSS Module via postcss-loader && a postcss plugin

plugin.js

export default postcss.plugin('name', (options = {}) => (css, result) => {
  // Get Imports && Exports somehow...

  // CSS Import
  result.messages.push({
    type: 'import' // required (Message Type)
    name: `CSS__IMPORT__${idx}` // Can be anything
    url: 'path/to/file' // Can be anything
    import () { // required (Message Stringifier)
       return `import ${this.name} from '${this.url}';`
    }
  })
  // CSS Export
  result.messages.push({
    type: 'export', // required (Message Type)
    name: `${name}`, // Can be anything
    value: 'Custom Export', // Can be anything
    export () { // required (Message Stringifier)
       return `export const ${this.name} = ${this.value};`
    }
  })
})

Answer 6 · 2018-01-18T16:10:47.000Z

It sounds like the Babel transform (either via a plugin or a macro) might be the best way forward. It also seems to be the favored solution in #3722 for implementing this for other file types.

Answer 7 · 2018-01-18T16:15:38.000Z

I think let's start with a userland solution based on babel-plugin-macros.
Something like

import toReactComponent from 'svgr/macro';

const Logo = toReactComponent('./icon.svg');

<Logo />

If that works we can revert the new feature, and instead document this as a supported way.

Answer 8 · 2018-01-18T16:17:34.000Z

@kentcdodds @threepointone Maybe you can give a rough idea of how to implement this macro?

Answer 9 · 2018-01-18T16:21:48.000Z

I guess it'll be similar to this: #1792 (comment)

Answer 10 · 2018-01-18T16:25:38.000Z

You got it @gaearon 👍 I think that's a good start to the solution for many problems like this.

So much for babel-plugin-macros being "experimental" 😉 😅

Answer 11 · 2018-01-18T16:26:43.000Z

It seems like we could avoid a hacky solution if file-loader and css-loader worked with ESM.
Then svgr-loader/webpack could provide two exports (one for URL and one for a component)

svgr-loader just needs to provide the metadata and url/file-loader needs to handle it based on a ESM-like 'format' (other loaders should be able to use this to)

and when SVG is imported through CSS, it would only get the URL. Is this something that should work in the future?

You can get the SVG URL (url()) from module.dependencies then in case it's an ESM Import (HarmonyImportSideEffectsDependency)

// import X__IMPORT from '${url}'
HarmonyImportSideEffectsDependency {
  name: 'X__IMPORT',
  request: '${url}',
  module: { <= file-loader
    ...
  }
}

Is it blocked on webpack 4?

Nope

But I see you want to go with babel instead... :)

Answer 12 · 2018-01-18T16:28:05.000Z

The main reason that I'm thinking of going with Babel is because the loaders still have to produce the build output (even if it ends up treeshaken). Seems like wasted effort when it's not trivial. The Babel solution neatly avoids this.

Answer 13 · 2018-01-18T16:28:14.000Z

(Still, thanks for all the context!)

Answer 14 · 2018-01-18T16:33:36.000Z

I'll take a crack at an implementation after work hours, think this is important enough to exist before 2.0 launches

Answer 15 · 2018-01-18T16:37:54.000Z

(I can sense the excitement in @threepointone's tone!)

Answer 16 · 2018-01-18T18:31:13.000Z

I'm experimenting with adding a macro to svgr. I've got the macro reading the SVG file and transforming it into a React component using svgr. I'm currently trying to figure out how to replace the macro function call with the generated code. I'm no babel expert so any help/suggestions would be welcome. Here's the code: https://github.com/iansu/svgr/tree/babel-macro

Answer 17 · 2018-01-18T18:35:21.000Z

Relevant code is in src/macro.js

Answer 18 · 2018-01-18T18:43:22.000Z

You'll find resources for learning about ASTs in the author docs:

Writing custom Babel and ESLint plugins with ASTs: A 53 minute talk by @kentcdodds
babel-handbook: A guided handbook on how to use Babel and how to create plugins for Babel by @thejameskyle
Code Transformation and Linting: A workshop (recording available on Frontend Masters) with exercises of making custom Babel and ESLint plugins

I hope that's helpful!

Answer 19 · 2018-01-18T18:45:46.000Z

Just noticed this:

import MacroError from 'babel-plugin-macros'

Should be:

import {MacroError} from 'babel-plugin-macros'

Answer 20 · 2018-01-18T18:51:00.000Z

Thanks. That was the last thing I added before I committed. It builds but I guess it will blow up when it tries to throw an exception.

I've read the babel handbook before. Just re-familiarizing myself with everything now. Thanks for the links!

Answer 21 · 2018-01-18T19:52:35.000Z

I just realized one major shortcoming of this approach is that we're effectively inlining SVGs. If you include the same SVG in your code twice it will end up inlined in the bundle twice. 😞

Answer 22 · 2018-01-18T19:56:28.000Z

Yup, that’s definitely an issue when using a macro as a ‘loader’, but can be worked around - write the generated component to a separate js file in the src directory, and import/require it from the original file.

Answer 23 · 2018-01-18T19:56:45.000Z

Yes, that's true. One way to get around this would be to put it in a module and import that module anywhere you like. Not quite as nice as a webpack solution.

Answer 24 · 2018-01-18T19:57:13.000Z

(We should probably make a helper to make macros-as-loaders simpler)

Answer 25 · 2018-01-18T19:58:01.000Z

That said, I'm not sure how often folks import the same svg in multiple places. I'm sure some people do, but for all the stuff I've worked with I only use svgs in one component.

Answer 26 · 2018-01-18T19:58:41.000Z

Yeah, the babel plugin that adds webpack loader syntax to import statements is starting to sound good. Although @threepointone's idea of saving the generated output and importing it is interesting.

Answer 27 · 2018-01-18T19:59:23.000Z

It’ll happen often enough Kent, once they use this more for loaders

Answer 28 · 2018-01-18T20:01:02.000Z

I think outputting files in src might be a bit shady.. Do you mean to put them somewhere else?

I would suggest only allowing one path to be “imported” once, and hard error if you attempt to “import” it again, suggesting instead to create a component.

That would require Babel plugin to keep the state between which SVG is imported where. Maybe not even possible.

Answer 29 · 2018-01-18T20:01:45.000Z

Yeah, you're right. We should probably come up with and document a good solution. What about a macro shopping with a loader and inlining the loader path? 0-config changes with totally different loaders 🤯

Answer 30 · 2018-01-18T20:02:27.000Z

Not sure I understand how that looks like from user perspective

Answer 31 · 2018-01-18T20:17:14.000Z

Outputting files in src isn’t too bad, but can agree it’s annoying. Another option is to put it into a tmp dir and add it to .gitignore. All this to work around the lack of any hooks into webpack.

(Inb4 webpack-macros)

Answer 32 · 2018-01-18T21:09:24.000Z

You could output the files to node_modules/.cache it something like that.

I'll show an example of what I mean when I get back to my keyboard Dan 👍

Answer 33 · 2018-01-18T21:20:04.000Z

@gaearon

I thought: what if we gave webpack more information without the user knowing about it? We could have an extra Babel transform that takes
import logoUrl, { ReactComponent as Logo } from './logo.svg';
and turns it into
// generated code
import logoUrl from './logo.svg';
import Logo from 'svgr-loader!./logo.svg';

I worked on an babel plugin that is doing that here. I wrote about my thoughts after implementing it in #3722

Answer 34 · 2018-01-18T22:12:58.000Z

Hey @FWeinb!

That's awesome! Thanks for doing that work. Here are my thoughts on.. eh... your thoughts 😅

I'll preface this by saying that it's my expectation that we wouldn't have a single babel plugin that does this for all loaders, but rather have a specific (probably unconfigurable) macro that does this for a single loader (like the svgr-loader). We could make one for every type you'd want. It would also be a macro, so actually using it would be a little different.

Ok, so my thoughts on your thoughts.

Configuration of the loaders in the webpack.config will be tedious

These macros would not be configurable. They'd come out of the box with pre-configured loaders (probably via a query string in the overloaded import statement I guess).

Some loaders will probably need some special options passed to work

That would be the job of the macro for the particular loader.

Maintaining a list of valid file extensions here is not optimal.

Because it would be a macro, that would be unnecessary because it's explicit.

I don't know if the proposed syntax is as intuitive as a I previously though

Syntax would be different.

So here's what I invision:

import toReactComponent from 'svgr/macro'
const Logo = toReactComponent('./icon.svg')
const ui = <Logo />

// would compile to:

import Logo from 'svg-react-loader?config=would&be=inlinedHere!./icon.svg'
const ui = <Logo />

To take it a step further, we could maybe have macros like this point to a loader that's not installed in the project, but rather installed as part of the macro, that way users don't have to have the loaders installed in addition to the macro:

import loadThing from 'my-loader.macro'
const thing = loadThing('./some.file')
console.log(thing)

// would compile to:

import loadThing from 'my-loader.macro'
console.log(loadThing('./some.file'))

import thing from '/User/username/full/path/to/node_modules/thing-loader/index.js?config=would&be=inlinedHere!./some.file'
console.log(thing)

I haven't tried this, but I think it should work and wouldn't be a terribly hard macro to write.

Answer 35 · 2018-01-18T22:14:22.000Z

I probably shouldn't mention it, but it is actually possible to configure babel-plugin-macros out of band (not in the .babelrc) (it's currently labeled as experimental). So these macros could actually be configurable if they needed to be.

Answer 36 · 2018-01-18T22:18:48.000Z

Hmm. I don't think a macro that generates a webpack import is much better than just a webpack import. Both lock you into a non-standard system.

The strong side of the macro is that it can run without webpack. Coupling it to webpack removes that strong side.

The strong side of Babel+webpack (but no macro) is that it “looks” seamlessly.

I think combining macro+webpack doesn’t give us benefits of either approach.

Answer 37 · 2018-01-18T22:23:33.000Z

I can get behind that @gaearon 👍

Any other suggestion for how to avoid the "duplicate content problem" that @iansu mentioned for people who use a macro that loads things like SVGs? Here's what I see as solutions we haven't marked off:

Saving it to a file (maybe in node_modules/.cache because that'll be in the .gitignore?)
Warning people of the problem (not certain how this would be accomplished honestly... Would have to store that state somewhere... I think a file would be the only way for that...)
Don't solve it at all and wait to see if it's a real problem? Maybe gzip will save us?

I think I'm leaning to 3 for now, with a fallback to 1. I'm not really excited about 2...

Answer 38 · 2018-01-18T23:08:16.000Z

Saving it to a file (maybe in node_modules/.cache because that'll be in the .gitignore?)

What if you run npm install or yarn and it deletes the cache? It feels flaky. I can’t easily describe in which scenarios it would break but I feel like it could.

Don't solve it at all and wait to see if it's a real problem? Maybe gzip will save us?

I doubt it, the minified variable names might be different. Also increasing parsed size is also bad.

Answer 39 · 2018-01-18T23:14:16.000Z

So are you suggesting solution 2? Or are you hoping we come up with another solution?

Anyone else have ideas?

Answer 40 · 2018-01-18T23:55:11.000Z

I'm leaning towards the approach in #3722 (comment). It provides good output with no gotchas, doesn’t require importing third-party tooling, doesn’t require unnecessary build-time work, and is flexible for future extension. I still think babel-plugin-macros has many useful potential applications but I feel like this is the case where the downsides of inlining and the pitfalls are too large.

Answer 41 · 2018-01-19T00:04:43.000Z

Agreed. For some reason I thought that solution had already been ruled out, but it seems like a viable solution 👍

Answer 42 · 2018-01-19T00:26:08.000Z

Let me know if I can help in managing the babel plugin. Should I create a PR and add the babel plugin to the CRA mono-repo?

Would be great to pick suitable webpack loaders for each file type for the PoC I just picked the first I could find.

I would need some help with some e2e tests running the plugin with webpack and verify that everything is working as expected.

Answer 43 · 2018-01-19T00:42:07.000Z

@FWeinb That sounds good. Initially we'd like to focus only on supporting SVG files. Eventually we'd like to make the loaders configurable (probably via babel-preset-react-app). Let me know if you have any questions or need help.

Answer 44 · 2018-01-19T12:12:44.000Z

@iansu Thanks!

I have no idea where this should go but I made a rough outline what I am planing to do.

Currently babel-plugin-named-asset-import has a hardcoded map of file type and loaders that can be mapped. To make this plugin more flexible in the future I would make this configurable and drop the defaults. Than each consumer of this plugin is responsible for defining which loader to use. For example babel-preset-react-app could use this plugin like this:

plugins: [
  [
    require('babel-plugin-named-asset-import').default,
    {
      loaderMap: {
        svg: { // file extention
          ReactComponent: 'svgr-loader', // map import variable to loader name
          url: 'url-loader'
        }
      }
    }
  ]
]

this would enable the transpilation of the inital example by @gaearon with a slight modification:

import logoDefault, { url as logoUrl,  ReactComponent as Logo } from './logo.svg';

turns into

import logoDefault from './logo.svg'; // depends on webpack configuration for 'svg'
import logoUrl from 'url-loader!./logo.svg';
import Logo from 'svgr-loader!./logo.svg';

I don't think that it would be good to allow babel-plugin-named-asset-import to change the behaviour of the default import. The defaults are configured by webpack without the need for transpilation.

Answer 45 · 2018-01-19T13:07:33.000Z

This sounds good to me.

Two potential tweaks:

We could consider not tying it to webpack, and instead providing a mapper for each type, e.g.
```
{
  svg: {
    ReactComponent: path => `svgr-loader!${./path}`
  }
}
```
This would decouple the transform from webpack. Not sure it's meaningful in any other context though. (Maybe we could use that in tests somehow?)
I agree about not changing defaults. But another thing worth researching/considering is what happens when webpack supports ESM for asset loaders. (See @michael-ciniawsky comments above, #3856 (comment)). Would file-loader ever want to have a named export that might be useful? Our plugin would make it impossible to access that. But maybe that won’t happen. Worth following up with a discussion in file-loader repo; I’d appreciate if you filed an issue there.

Answer 46 · 2018-01-20T17:26:19.000Z

@FWeinb Do I understand correctly that you intend to work on this, and then send a PR to incorporate your Babel plugin into our monorepo? It's important for us to be able to quickly tweak it.

Answer 47 · 2018-01-20T21:40:03.000Z

@gaearon
I did not know that it was this time critical. I have my last exam on the 25th so it would take probably at least till than for me to create a PR for this. So if someone else would do it I am totally fine with using the code of the existing babel plugin I created and tweaking it according to the discussion here.

Answer 48 · 2018-01-21T00:28:21.000Z

Oh, totally understood!

I don’t think it’s time critical but I’d like to fix this in the next alpha (or revert the first PR) so that people can try it in larger projects.

@iansu Maybe in this case you can start working on a PR based on @FWeinb’s plugin, and he can join at a later stage?

Answer 49 · 2018-01-21T01:20:58.000Z

That works for me. I'll get an initial PR ready for the SVG use case and then we can take it from there.

Answer 50 · 2018-01-21T17:42:16.000Z

I was in holiday, I try to catch up. Babel seems a good idea, Webpack does not provide as much flexibility.

@iansu do you need help?

Answer 51 · 2018-01-23T06:19:29.000Z

In my several projects, there are two kinds of SVG. One is icon-based which are exported from Adobe illustrator and the other is image-based (e.g., Logo) which are exported from Sketch.app.

I need to setup different configs of svgr for each SVG data source. The macro solution is supposed to be more configurable in this use case, and here is my full POC.

Logo componet: import one svg file with default config:

import toReactComponent from 'svgr.macro';
const Logo = toReactComponent('./logo.svg');

      ↓ ↓ ↓ ↓ ↓ ↓

const Logo = props => <svg width={116} height={28} viewBox="0 0 116 28" {...props}>
    <g fill="none" fillRule="evenodd">
      ...

Icon componets: glob multiple svg files with custom config:

const { DoneBlack, Autorenew } = toReactComponent(
  './material/*.svg',
  { icon: true, replaceAttrValues: ['#61DAFB=currentColor'] },
);

      ↓ ↓ ↓ ↓ ↓ ↓

const {
  DoneBlack,
  Autorenew
} = {
  "Autorenew": props => <svg height="1em" viewBox="0 0 24 24" width="1em" {...props}>
    ...
  </svg>,
  "DoneBlack": props => <svg height="1em" viewBox="0 0 24 24" width="1em" {...props}>
    ...
  </svg>
};

References

GitHub: https://github.com/evenchange4/svgr.macro
Example: https://github.com/evenchange4/svgr.macro-example
Demo: https://svgrmacro.netlify.com/

cc @iansu Do you still continue working on your branch into svgr repository?

Answer 52 · 2018-01-23T13:39:21.000Z

@evenchange4 we discourage using babel macros for svgs because they'll inflate the bundle size (if you import the same svg across multiple files). Do you have a solution for this besides importing and exporting out of a separate file that's used everywhere?

edit: this solution is still very cool & thank you for sharing

Answer 53 · 2018-01-23T13:44:12.000Z

I feel like it's okay for people who know what they're doing, but we won't be suggesting this as a default solution. It's cool that @evenchange4 made something flexible for his use case though :-)

Answer 54 · 2018-01-26T20:14:22.000Z

Why not using Webpack issuer? @ppozniak had the same problem (gregberge/svgr#43) and it looks like issuer is a good workaround.

Answer 55 · 2018-01-26T20:28:37.000Z

That sounds extremely elegant, @neoziro. Do you see any reasons why that wouldn't satisfy our use cases @gaearon?

Answer 56 · 2018-01-26T20:34:39.000Z

@Timer i actually have a branch laying around with test for css and svg inclusion, i'm adding the changes for issuer that @neoziro propose right now to see if it works 😄

Answer 57 · 2018-01-26T20:35:21.000Z

@neoziro But the Webpack issuer only provides the filename from which the import was made:

import A from './a.js'
A Condition to match against the module that issued the request. In the following example, the issuer for the a.js request would be the path to the index.js file.

Knowing that the request was made from index.js is not enough. We need to know what the import specifier (e.g. A) is to create the correct import.

Answer 58 · 2018-01-26T20:41:48.000Z

@FWeinb This would allow us to bypass the SVG loader if the request came from anything other than a .js file. So if a .css file is requesting the SVG then we just give it the URL. If a .js file is requesting the SVG then we provide the named export (as well as the default export).

Answer 59 · 2018-01-26T20:45:20.000Z

@iansu @neoziro
You are right, this would indeed solve the problem described in this thread. I was thinking about #3722 and the example provided by @gaearon.

Answer 60 · 2018-01-26T20:47:00.000Z

Yes, this solves this specific problem but doesn't necessarily address the larger issue of allowing multiple named imports for a variety of file types.

Answer 61 · 2018-01-26T20:59:04.000Z

the issuer solution only works for production config though, i think it's because of style-loader the issuer become .js?

Answer 62 · 2018-01-29T11:44:40.000Z

more info about last comment: i think it's because somehow the loaders are cached in dev, if i remove the import in the js file, and import svg from css that is also imported in that file it works.

so i think the babel plugin to transform the imports to webpack syntax still the best right now.

Answer 63 · 2018-06-03T16:15:38.000Z

Guys, can someone help clarify what's the current status with this?

Would there be a built-in svg->component integration (is there some work that could be done to achieve this?)
If not, recommended workaround to still get components by default?

Answer 64 · 2018-06-03T16:36:31.000Z

Hi @GuyKedem1 I believe what you need is already implemented in the next branch. You can follow instructions here to install it: #3815

CSS Loader (css-loader@next (⚠️ not released))

CSS => ESM (Formatting via css-loader)

CSS Plugin (extract-text-webpack-plugin@next (⚠️ not released))

file/url-loader