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

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

Closed
gaearon opened this Issue Jan 18, 2018 · 65 comments

Comments

Projects
@gaearon
Copy link
Member

gaearon commented Jan 18, 2018

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 gaearon added the issue: bug label Jan 18, 2018

@gaearon gaearon added this to Blocker in 2.0 Jan 18, 2018

@gaearon gaearon added this to the 2.0.0 milestone Jan 18, 2018

@gaearon

This comment has been minimized.

Copy link
Member

gaearon commented Jan 18, 2018

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?

@gaearon

This comment has been minimized.

Copy link
Member

gaearon commented Jan 18, 2018

@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?

@michael-ciniawsky

This comment has been minimized.

Copy link

michael-ciniawsky commented Jan 18, 2018

@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');
}

cssplugin

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

@michael-ciniawsky

This comment has been minimized.

Copy link

michael-ciniawsky commented Jan 18, 2018

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

@michael-ciniawsky

This comment has been minimized.

Copy link

michael-ciniawsky commented Jan 18, 2018

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};`
    }
  })
})
@iansu

This comment has been minimized.

Copy link
Collaborator

iansu commented Jan 18, 2018

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.

@gaearon

This comment has been minimized.

Copy link
Member

gaearon commented Jan 18, 2018

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.

@gaearon

This comment has been minimized.

Copy link
Member

gaearon commented Jan 18, 2018

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

@gaearon

This comment has been minimized.

Copy link
Member

gaearon commented Jan 18, 2018

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

@kentcdodds

This comment has been minimized.

Copy link
Contributor

kentcdodds commented Jan 18, 2018

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" 😉 😅

@michael-ciniawsky

This comment has been minimized.

Copy link

michael-ciniawsky commented Jan 18, 2018

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

@gaearon

This comment has been minimized.

Copy link
Member

gaearon commented Jan 18, 2018

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.

@gaearon

This comment has been minimized.

Copy link
Member

gaearon commented Jan 18, 2018

(Still, thanks for all the context!)

@threepointone

This comment has been minimized.

Copy link

threepointone commented Jan 18, 2018

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

@kentcdodds

This comment has been minimized.

Copy link
Contributor

kentcdodds commented Jan 18, 2018

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

@iansu

This comment has been minimized.

Copy link
Collaborator

iansu commented Jan 18, 2018

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

@iansu

This comment has been minimized.

Copy link
Collaborator

iansu commented Jan 18, 2018

Relevant code is in src/macro.js

@kentcdodds

This comment has been minimized.

Copy link
Contributor

kentcdodds commented Jan 18, 2018

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

I hope that's helpful!

@kentcdodds

This comment has been minimized.

Copy link
Contributor

kentcdodds commented Jan 18, 2018

Just noticed this:

import MacroError from 'babel-plugin-macros'

Should be:

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

This comment has been minimized.

Copy link
Collaborator

iansu commented Jan 18, 2018

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!

@iansu

This comment has been minimized.

Copy link
Collaborator

iansu commented Jan 18, 2018

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

@threepointone

This comment has been minimized.

Copy link

threepointone commented Jan 18, 2018

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.

@kentcdodds

This comment has been minimized.

Copy link
Contributor

kentcdodds commented Jan 18, 2018

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.

@FWeinb

This comment has been minimized.

Copy link

FWeinb commented Jan 20, 2018

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

@gaearon

This comment has been minimized.

Copy link
Member

gaearon commented Jan 21, 2018

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?

@iansu

This comment has been minimized.

Copy link
Collaborator

iansu commented Jan 21, 2018

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

@gaearon

This comment has been minimized.

Copy link
Member

gaearon commented Jan 21, 2018

Thanks!

@neoziro

This comment has been minimized.

Copy link

neoziro commented Jan 21, 2018

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?

@evenchange4

This comment has been minimized.

Copy link
Contributor

evenchange4 commented Jan 23, 2018

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?

@Timer

This comment has been minimized.

Copy link
Collaborator

Timer commented Jan 23, 2018

@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

@gaearon

This comment has been minimized.

Copy link
Member

gaearon commented Jan 23, 2018

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

evenchange4 added a commit to evenchange4/svgr.macro that referenced this issue Jan 23, 2018

@neoziro

This comment has been minimized.

Copy link

neoziro commented Jan 26, 2018

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

@Timer

This comment has been minimized.

Copy link
Collaborator

Timer commented Jan 26, 2018

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

@viankakrisna

This comment has been minimized.

Copy link
Contributor

viankakrisna commented Jan 26, 2018

@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 😄

@FWeinb

This comment has been minimized.

Copy link

FWeinb commented Jan 26, 2018

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

@iansu

This comment has been minimized.

Copy link
Collaborator

iansu commented Jan 26, 2018

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

@FWeinb

This comment has been minimized.

Copy link

FWeinb commented Jan 26, 2018

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

@iansu

This comment has been minimized.

Copy link
Collaborator

iansu commented Jan 26, 2018

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.

@viankakrisna

This comment has been minimized.

Copy link
Contributor

viankakrisna commented Jan 26, 2018

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

@viankakrisna

This comment has been minimized.

Copy link
Contributor

viankakrisna commented Jan 29, 2018

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.

@GuyKedem1

This comment has been minimized.

Copy link

GuyKedem1 commented Jun 3, 2018

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

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

This comment has been minimized.

Copy link
Collaborator

bugzpodder commented Jun 3, 2018

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

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

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