SVG's not scaling with Webpack 5

DanielHani commented 2 years ago

Hey and first of all thank you for this great plugin. I love it and use it for years now.

Are you planning any updates for newer Webpack versions? Im running into an issue with webpack 5.69.1.

The SVGs won't scale anymore. Everything else seems to be working fine. Currently im using a solution like this:

&::before { @include sprite('search', (color: red));

width: 512px;
height: 512px;
margin: -235px;
transform: scale(0.04);
content: "";
display: block;

}

But as you can see, this is far from pretty. It's difficult to resize and position them like this.

Best Regards

cascornelissen commented 2 years ago

I can't really imagine a patch release of webpack would cause such an issue but let's investigate.

If you mention a specific webpack version (5.69.1), does that mean it was working as expected with a previous version?
Are you able to provide a small reproduction repository that displays the issue at hand? Ideally with just a few simple sprites and as little webpack configuration as possible.

DanielHani commented 2 years ago

Looks like its a bug inside of the style-formatters/scss.js and not related to webpack at all.

After line: 50 there should be a svg.setAttribute('viewBox', symbol.getAttribute('viewBox')); but it's missing. Adding it fixed it for me and will fix it for others.

Do you mind committing this change soon?

Thanks mate :)

cascornelissen commented 2 years ago

Feel free to submit a PR with tests included, I'm still not getting how/what this will solve though as there is too little information so far to get a good grasp of the problem you're running into.

Especially because svg.setAttribute('viewBox', symbol.getAttribute('viewBox')); indicates that the viewBox property of the svg element would be set to equal that of the symbol. If that's your goal, then why not use the styles.keepAttributes option that's implemented just below the line you mentioned and includes almost the exact same line...

svg.setAttribute(attribute.name, attribute.value);

... where attribute.name would resolve to 'viewBox' and attribute.value would resolve to the symbol's viewbox.

https://github.com/cascornelissen/svg-spritemap-webpack-plugin/blob/5e73ae55f0f6bc29ccee279f54d312a60561514b/lib/style-formatters/scss.js#L53-L61

DanielHani commented 2 years ago

I can try it that way. I found out that in an older version you had that line and after a big rework it was gone. Looks more like a mistake to me.

The generated SVG needs to have a viewBox. If it doesn't, it doesn't scale. Because of it being a must have for an svg it should always be implemented like it was in the older versions of this plugin.

Edit: Heres a link to another discussion. People had the same problem there: https://github.com/svg/svgo/issues/1128#issuecomment-628208565 This comment shows exactly what i was experiencing.

cascornelissen commented 2 years ago

This line was "removed" as part of https://github.com/cascornelissen/svg-spritemap-webpack-plugin/pull/121. It wasn't actually removed; it was made more generic and in a following commit, https://github.com/cascornelissen/svg-spritemap-webpack-plugin/commit/6620316c79ff24caa8c882013491b5000cd5f8eb, I added the styles.keepAttributes option to ensure the change was backwards compatible so definitely not a mistake.

The generated SVG needs to have a viewBox. If it doesn't, it doesn't scale. Because of it being a must have for an svg it should always be implemented like it was in the older versions of this plugin.

This is true in your setup but you have to take into account that different people use this plugin in different ways, which is the reason for the change to ensure backwards compatibility and more importantly; the reason it's an option that people like you can enable to get to the end-result they're looking for.

Since I'm 99% sure setting the styles.keepAttributes option to true is the solution to what you're looking for, I'm closing this issue. Feel free to re-open/comment if this is not the case, of course!

The only minor issue that this could have caused (which might be the thing you're experiencing) is that this was a change that could be seen as unexpected. Unfortunately, there's not much to do about it at this point in time. Besides, the fact that the change was made 1,5 years ago and this is, AFAIK, the first issue about it sort of suggests to me not that many people saw it as an issue.

DanielHani commented 2 years ago

keepAttributes does help.

None the less i think viewBox is in most cases crucial and should not be removed by default. Instead an option to remove it if needed would be better. Just an idea.

I've experienced this "issue" a year ago, so i sticked to the older version as i didn't have time to investigate it.

I found someone who stumbled upon this before: https://github.com/cascornelissen/svg-spritemap-webpack-plugin/issues/173

Or maybe mention it at least in the readme? That could save ppl some time i think.

It's yours to decide. Keep the good work up. :)

cascornelissen commented 2 years ago

None the less i think viewBox is in most cases crucial and should not be removed by default. Instead an option to remove it if needed would be better. Just an idea.

I'm not disagreeing with you, it might indeed make sense but I can't make that change and keep it backwards compatible and I don't see the value of creating a major release just to flip that option.

If you have an idea about where to add it in the documentation to make this clearer, feel free to submit a PR. I'm always happy to improve documentation for users.

cascornelissen / svg-spritemap-webpack-plugin

SVG's not scaling with Webpack 5 #187