mrsum / webpack-svgstore-plugin

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

upgrade to webpack2 and modern class based structure #119

Closed nadavsinai closed 7 years ago

nadavsinai commented 7 years ago

well. it doesn't run in node 0.12... works fine in node 4+ why should anyone use node 0.12?

nadavsinai commented 7 years ago

rebased, taking your latest update-deps commit except of using webpack 2 API...

ryan-codingintrigue commented 7 years ago

Doesn't work for Webpack 2.2.0-rc.0.

Cannot read property 'properties' of undefined /node_modules/webpack-svgstore-plugin/src/svgstore.js:52:22

ryan-codingintrigue commented 7 years ago

I did a bit of research into this. It appears that an expression is not being sent to the analyzeAst method so it never gets visibility of the object literal.

I don't know much about Webpack's parsing, so I couldn't figure out how to locate the statement rather than the variable declaration through the parser interface, so I wrote a callback for all statements and filtered them out based on name. Seems to work fine:


//parser.plugin('var __svg__', analyzeAst);
parser.plugin('statement', function(expr) {
  if(!expr.declarations || !expr.declarations.length) return;
  const thisExpr = expr.declarations[0];
  if(thisExpr.id.name === "__svg__") {
    return analyzeAst.call(this, thisExpr);
  }
  return;
});
`
mrsum commented 7 years ago

Hi @ryan-codingintrigue, great job. So, what about webpack 1.x?

ryan-codingintrigue commented 7 years ago

@mrsum Technically my code works for Webpack 1.x, replacing parser with compiler.parser. However the problem lies in the reference to parser which changes between 1.x & 2.x:

data.normalModuleFactory.plugin('parser', (parser, options) => {
    // This is 2.x syntax.
    // data.normalModuleFactory.plugin exists in 1.x, but callback does not trigger
});

As you can see, this makes it difficult to distinguish between 1/2 via feature detection. The only thing Webpack 2 does differently is return an object literal from compiler.parser so you could test for compiler.parser.toString() === "[Object object]" but that's brittle.

nadavsinai commented 7 years ago

@ryan-codingintrigue thanks a bunch, you helped me much with your 'statement' plugin. @mrsum I made this a semver major - It works great but it doesn't work with webpack 1.x... is a new major a viable solution for you? I don't think that supporting webpack 1.x has any future as we are near 2.2.0 release. but I am speaking from my own viewpoint here, what do you guys say?

mrsum commented 7 years ago

@nadavsinai @ryan-codingintrigue Hi guys! Looks great! So, i wanna test your MR, and if everything is ok, will merge it ASAP!

nadavsinai commented 7 years ago

10x @mrsum , we still need to update the readme to reflect support of webpack >2.2.0 and node > 6, i'll be happy if you can do it yourself - i'm quite busy today...

nadavsinai commented 7 years ago

about the spaces - I can do it- but it won't be this week, about webpack 1.x - sorry but this is not in my priority... you can either make it a major version with these requirements or create a destination branch, I'll make a new PR for this branch and so you could keep working on it to add the webpack 1.x fallback code.

mrsum commented 7 years ago

@nadavsinai don't worry, review info just for me )

nadavsinai commented 7 years ago

tested with webpack 2.2.0 final today -- works well

finom commented 7 years ago

Any news?

irisSchaffer commented 7 years ago

Could it be that with this branch you need to call your mark __svg__ instead of also allowing __sprite__ (/ __svgstore__ / __svgsprite__)? Now that I use this branch, I had to rename it to __svg__...

ryan-codingintrigue commented 7 years ago

@irisSchaffer Good spot - I didn't realise there were multiple options for the mark. We should probably update this branch to allow for as much config backwards-compatibility as possible.

lgordey commented 7 years ago

I actually agree with @nadavsinai @mrsum We can up major version and add fallback for 1.x later. Why not?

rbirbeck commented 7 years ago

Hey, this branch is awesome!

I'm upgrading to Webpack 2.2 from Webpack 1.x and SvgStore 2.2.2 and this branch resolves almost all of my issues.

I am noticing one small difference between the old 2.2.2 functionality and this branch. I assume it changed some point in the past (unrelated to this branch), but I haven't been able to track down the change.

All of the SVGs in our repo are named [something].min.svg and we used to reference them as icon-something. Using this branch the SVGs are now getting an id of icon-something-min.

Is there a setting I am missing to avoid adding -min to the id?

lgordey commented 7 years ago

Ok, don't worry. I'll check this and we'll up the major version soon, in a few days I think.

lgordey commented 7 years ago

Try to use v4.0.0