jhamlet / svg-react-loader

Webpack SVG to React Component Loader
MIT License
559 stars 82 forks source link

Cleanup svg #40

Closed teameh closed 8 years ago

teameh commented 8 years ago

Awesome loader, exactly what I was looking for moving from my old gulp setup :).

The only thing I'm still looking for is a way to cleanup the SVG with something like https://github.com/svg/svgo. Do you know if this is currently possible? I could work on a PR if not. Let me know.

teameh commented 8 years ago

Okay, my bad. I think I did not look well enough in the output to see that you already do some cleanup. I think I overlooked this because it seems the loader adds some extra stuff as well:

<svg version="1.1" id="Icons" xmlns="http://www.w3.org/2000/svg" data-svgreactloader="[[&quot;http://www.w3.org/2000/svg&quot;,&quot;xlink&quot;,&quot;http://www.w3.org/1999/xlink&quot;],[&quot;http://www.w3.org/2000/svg&quot;,&quot;space&quot;,&quot;preserve&quot;]]" x="0px" y="0px" width="21px" height="21px" viewBox="0 0 21 21" enable-background="new 0 0 21 21" xlink="http://www.w3.org/1999/xlink" space="preserve">
    <path d="8 0 1.4.6 1.4 1.4 0 .7-.6 1.4-1.4 1.4zm ... "></path>
</svg>

Is this supposed to happen?

I tried ta add svgo btw, which resulted in the folowing output:

<svg xmlns="http://www.w3.org/2000/svg" width="21" height="21" viewBox="0 0 21 21">
    <path d="8 0 1.4.6 1.4 1.4 0 .7-.6 1.4-1.4 1.4zm ... "></path>
</svg>

I can imagine that even with some sanitising in place, it might be handy to replace that for svgo. Although that means adding another dependency to the project, you do get the benefit of regular patches to svgo (and not having to maintain the sanitise code yourself) plus you could support used defined svgo options (in the webpack config?) so users can benefit from all svgo options.

Here's the patch I used to add svgo, also see https://github.com/tiemevanveen/svg-react-loader/commit/d893659a253c3388f5a3c0166d1689a9f74ea5b4

--- a/index.js
+++ b/index.js
@@ -9,6 +9,8 @@ var assign   = require('lodash/assign');
 var keys     = require('lodash/keys');
 var partial  = require('lodash/partial');
+var SVGO     = require('svgo');

 function readTemplate (callback, filepath) {
     fs.readFile(filepath, 'utf8', function (error, contents) {
         if (error) {
@@ -20,7 +22,17 @@ function readTemplate (callback, filepath) {

 function parseXml (callback, source) {
     var xmlParser = new xml2js.Parser();
-    xmlParser.parseString(source, callback);
+
+    var svgo = new SVGO();
+    svgo.optimize(source, function(result) {
+        if (result.error) {
+            console.log('SVGO failed', result.error);
+            // fall back on source
+            xmlParser.parseString(source, callback);
+        }else{
+            xmlParser.parseString(result.data, callback);
+        }
+    });
 }
tera-sinube commented 8 years ago

@tiemevanveen Apologies for not getting to this earlier.

Those properties are added because, at the time, React didn't handle namespaced attributes, so react-svg-loader placed them in a data-svgreactloader attribute as JSON to be added when the componentDidMount.

So, removing those will break the loader for those using older versions of React.

Also, there is no point in having svg-react-loader doing svgo's work. There is already a loader for svgo you can use that as part of your loader chain, or as a pre-loader.

In addition, the next version of svg-react-loader is in beta, and I want to stop supporting anythig before that.

Again, thanks for trying to contribute and apologies for not replying before you did the work, but these changes are not necessary.

teameh commented 8 years ago

No problem.

Also, there is no point in having svg-react-loader doing svgo's work. There is already a loader for svgo you can use that as part of your loader chain, or as a pre-loader.

Check. That makes sense. How would I do that, could you give me an example?

In addition, the next version of svg-react-loader is in beta, and I want to stop supporting anythig before that.

Oh so the master branch is not the next release.. That's not so handy if it's not mentioned in the docs. User cloning the repo will always checkout master when wanting to contribute.

jhamlet commented 8 years ago

@tiemevanveen good point -- was just thinking I should add that to the current versions reame...

teameh commented 8 years ago

Would have saved me fixing the tests ;) https://github.com/jhamlet/svg-react-loader/pull/42

teameh commented 8 years ago

Anyway. Keep up the good work, looking forward to v0.4!

jhamlet commented 8 years ago

done

teameh commented 8 years ago

Check. That makes sense. How would I do that, could you give me an example?

Got it :)

{
    test: /\.svg$/,
    loaders: ['svg-react-loader', 'svgo-loader'],
    exclude: /node_modules/
}