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

Fix broken SVGs - issue #4066 / #4180 #4260

Merged
merged 1 commit into from
Oct 3, 2018
Merged

Fix broken SVGs - issue #4066 / #4180 #4260

merged 1 commit into from
Oct 3, 2018

Conversation

Brian-McBride
Copy link
Contributor

The webpack currently uses both file-loader and svg-url-loader.

file-loader will handle .svg files just fine. svg-url-loader is causing images to not load properly. This might be identified by @cainlevy in issue 4180:

svg-url-loader is not drop-in compatible with url-loader.

Unless there is a compelling reason to use svg-url-loader instead of just leveraging the currently utilized url-loader, this change seems to fix the issues and can result in removing the dependency on svg-url-loader from the repo completely.

Issue:

What I did

How to test

Is this testable with Jest or Chromatic screenshots?
Does this need a new example in the kitchen sink apps?
Does this need an update to the documentation?

If your answer is yes to any of these, please make sure to include it in your PR.

For maintainers only: Please tag your pull request with at least one of the following:
["cleanup", "BREAKING CHANGE", "feature request", "bug", "documentation", "maintenance", "dependencies", "other"]

The webpack currently uses both file-loader and svg-url-loader.

file-loader will handle .svg files just fine. svg-url-loader is causing images to not load properly. This might be identified by @cainlevy in issue 4180:
> svg-url-loader is not drop-in compatible with url-loader.

Unless there is a compelling reason to use svg-url-loader instead of just leveraging the currently utilized url-loader, this change seems to fix the issues and can result in removing the dependency on svg-url-loader from the repo completely.
@storybook-safe-bot
Copy link
Contributor

Fails
🚫

PR is not labeled with one of: ["cleanup","BREAKING CHANGE","feature request","bug","documentation","maintenance","dependencies:update","dependencies","other"]

Generated by 🚫 dangerJS

@Brian-McBride Brian-McBride changed the title Fix for issue #4066 / #4180 Fix for issue #4066 / #4180 ["bug","dependencies"] Oct 2, 2018
@igor-dv
Copy link
Member

igor-dv commented Oct 2, 2018

There was this PR #3221 that changed it to svg-url-loader in favor of a file size optimization.
CC @wuweiweiwu

Can't we fix an actual problem though?

@cainlevy
Copy link

cainlevy commented Oct 2, 2018

if svg-url-loader is valuable, then i would recommend configuring it with noquotes: true.

#4180

@Brian-McBride
Copy link
Contributor Author

So, the suggestion is to change line 44 in this file:
https://github.com/storybooks/storybook/blob/b2b73596f9dd97b4ba8c03340bd36f868e836772/lib/core/src/server/config/webpack.config.default.js#L44 to:

    loader: require.resolve('svg-url-loader?noquotes'),

@cainlevy Have you tested this?

@cainlevy
Copy link

cainlevy commented Oct 2, 2018

Effectively, yes. I patched the webpack config in my app with:

module.exports = (baseConfig, env, config) => {
  const svgLoader = config.module.rules
    .find((r) => r.loader && r.loader.indexOf('svg-url-loader') !== -1);
  svgLoader.query = {noquotes: true};
}

@igor-dv igor-dv changed the title Fix for issue #4066 / #4180 ["bug","dependencies"] Fix broken SVGs - issue #4066 / #4180 Oct 3, 2018
@ndelangen ndelangen merged commit 5bf4acd into storybookjs:master Oct 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants