mrsum / webpack-svgstore-plugin

Simple svg-sprite creating with webpack
https://www.npmjs.com/package/webpack-svgstore-plugin
200 stars 92 forks source link

Fix onDOMLoad svgXHR call #94

Closed dmitriid closed 8 years ago

dmitriid commented 8 years ago

Fix #75

KhramtsovMaksim commented 8 years ago

Please add this changes asap.

mrsum commented 8 years ago

@dmitriid @KhramtsovMaksim try 3.0.0-alpha.2

mrsum commented 8 years ago

@dmitriid Hi. fixed. You can override your svgXHR function

KhramtsovMaksim commented 8 years ago

Thank you!

dmitriid commented 8 years ago

How and where can I override this function?

mrsum commented 8 years ago

@dmitriid put function at your project and require that file or call function with svg argument for example

var __svg__ = { path: './assets/svg/**/*.svg', name: 'assets/svg/[hash].logos.svg' };
var spriteLoad = function(options) {
 $.ajax(options.filename)
}
spriteLoad(__svg__);
dmitriid commented 8 years ago

Aha. So __svg__ is a magical variable (this is not immediately apparent from the docs). One would assume that it's the svgXHR that does that. You have to look into the code of the function itself to see that it's not the case and that a variable is silently being overwritten by the plugin.

I'm am extremely sceptical about such a decision, to say the least.

Is previous behaviour available in any form (where you could specify a chunk to prepend the function to and not depend on magical variables somewhere in code)?

mrsum commented 8 years ago

@dmitriid svg – is the variable wich will modified inside webpack compile. As you know, import and require() commands are replace by ConstPlugin, i've made same logic with special variables. That is better than last implementation, cause you can create a lot of other sprites with only one SvgStore instance in webpack.config.js

Another case is opportunity of override svg ajax function, also AST compiler in webpack is much better than a lot of regexp and magic like in previous release. So, try to use it.

Best regards.

dmitriid commented 8 years ago

I will obviously try to use it. And ability to create multiple sprites is very nice.

However, magical variables that are silently overwritten? This is definitely obviously bad. import and require are known throughout JS world as:

Introducing not one, but several magical variables? That are silently overwritten? Erm... Nope.

mrsum commented 8 years ago

@dmitriid maybe, but a lot of plugins use same logic. for example https://github.com/js-next/react-style-webpack-plugin/blob/master/lib/entry.js#L10

dmitriid commented 8 years ago

It looks like it creates a different temporary variable, not relying on a pre-existing magic variable.