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

2.0 alpha #91

Closed
kisenka opened this issue Apr 23, 2017 · 21 comments
Closed

2.0 alpha #91

kisenka opened this issue Apr 23, 2017 · 21 comments

Comments

@kisenka
Copy link
Contributor

kisenka commented Apr 23, 2017

Hi all! I am preparing to release big update, it's already accessible as svg-sprite-loader@2.0.0-alpha.4.

Please read carefully about upcoming breaking changes, new features and bugfixes.

Breaking changes

Node.js >= 6 required

¯_(ツ)_/¯

If you think that Node.js < 6 should be supported, please vote up in this issue.

Targeting Webpack 2

¯_(ツ)_/¯

If you think that Webpack 1 should be supported, please vote up in this issue.

Loader should always be configured with plugin, otherwise error is thrown.

Reason: architectural changes.
Migration: just add a plugin in your webpack config

// webpack.config.js
const SpriteLoaderPlugin = require('svg-sprite-loader/plugin');

...

{
  plugins: [
    new SpriteLoaderPlugin()
  ]
}

name config option renamed to symbolId

Reason: more obviously.

regExp config option was removed

Reason: symbolId should cover most of naming cases.

prefixize config option was removed

Reason: all symbol elements isolated by default.

angularBaseWorkaround config option was removed

Reason: Angular introduced ability to remove the base tag requirement, see correspondent issue comment. If you still need it check angular-svg-base-fix.

Runtime has changed

Instead of symbol id runtime module now returns an object (class instance actually) which contains id, viewBox and content fields. See SpriteSymbol class and runtime generator.

Reason: make runtime more flexible, also it was requested in #32.
Migration:

// old
import symbol from './image.svg';
const rendered = `
<svg>
  <use xlink:href="${symbol}" />
</svg>`;


// new
import symbol from './image.svg';

// now .viewBox is available
const rendered = `
<svg viewBox="${symbol.viewBox}">
  <use xlink:href="#${symbol.id}" />
</svg>`;

If you think that loader should provide compatibility in this case, please vote up in this issue.

Features, improvements and bugfixes

Auto configuring

Some magic now happens by default, viz:

Sprite generator

  • Sprite/symbol generator was moved in separate project (svg-baker) and fully reworked. It's incredible customizable now. Customization via plugin coming soon.

Client runtime

Server side rendering

Extract sprite/sprites as separate file/files

TODO

  • More tests and examples.
@eugene1g
Copy link

Great, trying to test it out now. Everything seems to compile well, but the sprite is not injecting into the body as with v1. Do I need to make an explicit call to trigger this now?

@kisenka
Copy link
Contributor Author

kisenka commented Apr 23, 2017

@eugene1g sprite should injects automatically. Which webpack version is used?

@eugene1g
Copy link

eugene1g commented Apr 23, 2017

Webpack2.

 {
      test: /\.svg$/,
      include: [faModulesSrcDir + "/fa-icon"],
      use: [
        {
          loader: "svg-sprite-loader",
          options: {
            symbolId: "[name]_[hash:base64:8]",
            spriteModule: "svg-sprite-loader/runtime/browser-sprite.build"
          }
        },

        "svgo-loader"
      ]
    }

If I leave out spriteModule, then nothing gets injected. If I manually specify it, then it get injected but .stringify() is called before any symbols get populated and the resulting SVG has no nodes

@eugene1g
Copy link

Ok two separate notes -

  1. The default/parent spriteModule svg-sprite-loader/runtime/sprite.build does not have DOM mounting logic, so it makes sense that it doesn't get injected. Hence forcing it to use browser-sprite fixes that problem.

  2. I include all JS scripts at the end of the page, so when browser-sprite gets loaded it immediately mounts because document.body exists but before my require("icon.svg") code gets executed (because webpack does not guarantee the order of execution of every module). Therefore, the sprite gets mounted with no symbols in it, as none are populated at runtime yet. If I force the mount to happen on DOMContentLoaded, then all symbols are populated as expected.

@kisenka
Copy link
Contributor Author

kisenka commented Apr 23, 2017

@eugene1g I need your setup for case №1, could you please create a gist/repo with minimal setup which can reproduce described behaviour.

@kisenka
Copy link
Contributor Author

kisenka commented Apr 23, 2017

If I force the mount to happen on DOMContentLoaded, then all symbols are populated as expected

You're right. Seems like logic if (document.body) { sprite.mount(document.body, true); } should be removed to avoid such cases.

@eugene1g
Copy link

Ok I made a simple demo, but there the injection works as described - so something must be weird within my main environment. Only the document.body point was illustrated - have to dig in a bit more. I can't do it now so will have to triage during the week :/

@kisenka
Copy link
Contributor Author

kisenka commented Apr 23, 2017

Okay, svg-sprite-loader@2.0.0-alpha.2 is published, now browser sprite only renders when DOMContentLoaded happens.

@eugene1g
Copy link

Thanks! Fixes my use-case.

Perhaps there is merit in optimistic execution - especially if somebody is loading your script async, or on-demand, or lazily later on. In those cases DOMContentLoaded won't ever fire, and it's better to execute asap. Perhaps it's worth replacing that listener with domready - it will execute the mount for both use-cases, and the lib is small and well-tested across browsers.

@kisenka
Copy link
Contributor Author

kisenka commented Apr 23, 2017

svg-sprite-loader@2.0.0-alpha.3 with domready is published :)

@kisenka
Copy link
Contributor Author

kisenka commented Apr 23, 2017

Also browser-sprite example added.

@rocking42
Copy link
Contributor

Hey @kisenka, I've just submitted a PR #96

It's just related to where Sprite is imported in plugin.js and resolving the pathname as unix doesn't mind this but when we used a Linux system it caused an issue.

Love the work and let me know if there's anything I need to fix with my PR

@kisenka
Copy link
Contributor Author

kisenka commented Apr 24, 2017

@rocking42 merged, published as 2.0.0-alpha.4

@alexandernanberg
Copy link

I have a use-case which seems to be broken with the alpha builds. I've updated my webpack config to match the browser-sprite example.

const files = require.context('./glyphs', false, /.svg$/)
files.keys().forEach(files)

But when I run webpack I get this error.

image

All the source code can be found here (haven't pushed the alpha changes)

Should I use a different approach or is it a bug in the alpha versions?

Thanks!

@kisenka
Copy link
Contributor Author

kisenka commented Apr 26, 2017

@alexandernanberg there is an error in your webpack.config.js, you should replace module: { loaders: [ ... ] } to module: { rules: [ ... ] }

@alexandernanberg
Copy link

@kisenka Ahh... Weird that I haven't been affected by that until now. Thanks anyway! 😄

@eugene1g
Copy link

webpack 2 is surprisingly backwards compatible and as part of that translates all loaders to rules behind the scenes - that's why your config file (from webpack1 days!) was still working without issues.

@oliverturner
Copy link

Does anyone else have this error?

(node:38229) Warning: a promise was created in a handler at {path to}/node_modules/svg-sprite-loader/lib/plugin.js:95:23 but was not returned from it, see http://goo.gl/rRqMUw
    at Function.Promise.cast ({path to}/node_modules/bluebird/js/release/promise.js:196:13)

Using svg-sprite-loader@2.0.0-alpha.4 on Node v7.9.0

@kisenka
Copy link
Contributor Author

kisenka commented Apr 27, 2017

@oliverturner this is not an error, but I'll fix it.

@oliverturner
Copy link

@kisenka thank you! Much appreciated 👍

kisenka added a commit that referenced this issue Apr 28, 2017
@kisenka
Copy link
Contributor Author

kisenka commented Apr 28, 2017

🎉 2.0 is out, please read migration guide & overview.

@kisenka kisenka closed this as completed Apr 28, 2017
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

5 participants