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

How to only inject symbols that are in use #45

Closed
wmertens opened this issue Jun 23, 2016 · 17 comments
Closed

How to only inject symbols that are in use #45

wmertens opened this issue Jun 23, 2016 · 17 comments

Comments

@wmertens
Copy link
Contributor

wmertens commented Jun 23, 2016

when using style-loader, the loaded modules determine the css.

However, with sprite-loader, the common use is to have an Icon component that renders a component based on a list of glyphs. This means that when you load the Icon component, all the glyphs get injected in the file.

I would like to know the best technique so that only active glyphs are loaded, specifically for React ≥v0.14.

How about a singleton <Sprites/> component that you put somewhere in your app, which renders the current <symbol/> set, and then instead of <use href… you use <Sprite id={…}/> which, behind the scenes, updates the Sprites element based on refcounting in constructor and componentWillUnmount.

Furthermore, since you can put the sprites at the bottom of the page (proof), I believe that will also solve server side rendering since it will be stringified after all the icons were referenced.

@kisenka
Copy link
Contributor

kisenka commented Jun 23, 2016

@wmertens you want something like this - https://github.com/webpack/style-loader#reference-counted-api?

@wmertens
Copy link
Contributor Author

Yes I suppose that can be used for it.

I just realized that #32 and that refcount API can be implemented in a backwards compatible way by not returning a string id, but an object that returns the id for .toString() and .valueOf(), and also has .use(), .unuse() and .viewBox

@kisenka kisenka self-assigned this Jun 23, 2016
@wmertens
Copy link
Contributor Author

wmertens commented Jul 19, 2016

Ok, I'm starting to need this, so I'd like your thoughts on the proper approach. This is what I think needs to be done:

  • Create <Sprites/> React component that returns the <svg/> with all the sprites (instead of using document.write). Add a query flag useSprites: true to the loader to enable it.
  • Create <Sprite id={...}/> React component that makes a <svg><use .../></svg>.
    • It could have a noSvg attribute to just return the <use/> so it can be used inside <svg/>.
  • Add refcounting API, and change the above components to use it. The output of the loader should be an object with:
    • .toString(): returns sprite id
    • .use(), .unuse(): to be called from the <Sprite/> component's constructor and ...WillUnmount
    • .Sprite: pre-bound <Sprite/>component

So example usage would be

import {Sprite, Sprites, initSprites} from 'svg-sprite-loader'
import star from './star.svg'

const App = () => {
  initSprites() // only needed for server side rendering
  return (<div>
    <Sprite id={star}/><Sprite id={star}/><Sprite id={star}/>
    <Sprites/>
  </div>)
}

or with the pre-bound Sprite:

import {Sprites, initSprites} from 'svg-sprite-loader'
import {Sprite: Star} from './star.svg'

const App = () => {
  initSprites() // only needed for server side rendering
  return (<div>
    <Star/><Star/><Star/>
    <Sprites/>
  </div>)
}

Thoughts? I realize this is only for React, so maybe the components should be in a separate repo?

@wmertens
Copy link
Contributor Author

With the loader-returns-object approach, the <symbol/> source can be part of that object, so that it can be passed to <Sprites/> via .use().
That way, any implementation is possible, and the loader is only responsible for creating the symbol from the .svg file.

@wmertens
Copy link
Contributor Author

wmertens commented Jul 19, 2016

Continuing the thinking: Make a react-sprite-loader which exports <Sprites/> and initSprites() apart from being a webpack loader, and which expects objects with id and symbol (the symbol source as a string) as input.

Tell webpack to chain it with svg-sprite-loader, something like:

{
  test: /icons\/.*\.svg/,
  loaders: ['react-sprite', 'svg-sprite?object&loadOnly'],
}

and in the code

import {Sprites, initSprites} from 'react-sprite-loader'
import Star from './icons/star.svg'

const Component = (props) => (
  <div>
    <Star/><Star/><Star/>
  </div>
)

const App = (props) => {
  initSprites() // only needed for server side rendering
  return (
    <div>
      <Component/>
      <Sprites/>
    </div>
  )
}

So then svg-sprite-loader would only need to return plain objects with id and symbol (and viewBox). The id could be a hash of the symbol, which would cause de-duplication.

@kisenka
Copy link
Contributor

kisenka commented Jul 19, 2016

I'd like to @princed answered your question, because I am not good in React. But I fully agree with suggestion to return object (or class instance) from sprite-loader.

@princed
Copy link
Contributor

princed commented Jul 21, 2016

Actually we use a bit different approach like described in the very end of the README, so icon component doesn't have to require all the icons.

I don't think we should have something react specific. We'd better have common SSR solution.

And we could use counting API to remove inactive defs from DOM, however it could lead to more complex code due our workarounds.

@wmertens
Copy link
Contributor Author

@princed how does that work? All the icons are required statically, no? And then they all get added to the DOM, even when they are not in use?

@wmertens
Copy link
Contributor Author

wmertens commented Aug 2, 2016

@princed @kisenka ping

@princed
Copy link
Contributor

princed commented Aug 3, 2016

@wmertens Yes, all the svgs are required statically in components that use icon component.
And yes, all the required svgs get added to the DOM, even when they are not in use.

Why does it actually concern you?

@radiovisual
Copy link

And yes, all the required svgs get added to the DOM, even when they are not in use.

This is a concern for me, because it is adding unused elements to the DOM. The projects I work on try to avoid dumping unused data to the DOM to keep the DOM/templates clean and lightweight. Also, in certain situations, SVG's can come with security risks, so in a paranoid world: it's best to limit SVG's to only those you require.

I think that many of the ideas presented by @wmertens are really great, and I think providing a react-specific loader option is a cool idea, or breaking the react-ready component out to it's own repo.

@princed
Copy link
Contributor

princed commented Aug 3, 2016

And we could use counting API to remove inactive defs from DOM, however it could lead to more complex code due our workarounds.

Does this style-loader-like solution seem appropriate to you?

@kisenka
Copy link
Contributor

kisenka commented Aug 3, 2016

@radiovisual

it is adding unused elements to the DOM

Just don't import unnecessary SVGs in your code

@princed
Copy link
Contributor

princed commented Aug 17, 2016

And we could use counting API to remove inactive defs from DOM, however it could lead to more complex code due our workarounds.

Does this style-loader-like solution seem appropriate to you?

@wmertens @radiovisual I've just wanted to be sure that it's way to go for you.
Because I'd rather keep this loader framework-agnostic despite we use React for our products too.

@radiovisual
Copy link

@princed , I am all for an option that keeps things framework agnostic, but lets you easily drop into a React environment when needed. Sounds good to me, so I like @wmerten's react-style-loader idea.

@princed
Copy link
Contributor

princed commented Aug 18, 2016

I'm just curious why some may want to deal with this stuff manually although no one does the same with styles from style-loader.

@kisenka
Copy link
Contributor

kisenka commented Apr 28, 2017

New loader version adds ability to specify custom runtime generator. Please check default generator and custom generator implementation example.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants