bholloway / resolve-url-loader

Webpack loader that resolves relative paths in url() statements based on the original source file
563 stars 71 forks source link

add option to keep query string in CSS url #21

Closed kroko closed 8 years ago

kroko commented 8 years ago

Currently resolve-url-loader strips off all query strings for CSS URLs. There is a reason why coder would add query or hash to the end of URL, we should keep it.

For example. This is undesirable (meaning breaking :) ) when url is eot font with dummy query that is needed for IE. Sure, it is IE8, however we still want to get "update your browser" message using nice typeface. And Webpack is not only for top edge, you can pack ES3 apps with it :)

Consider

@font-face {
  font-family: 'myfontfamily';
  src: url('./MyFont.eot'); /* IE9 Compat Modes */
  src: url('./MyFont.eot?#iefix') format('embedded-opentype'), /* IE6-IE8 */
       url('./MyFont.woff') format('woff'), /* Modern Browsers */
       url('./MyFont.ttf')  format('truetype'), /* Safari, Android, iOS */
       url('./MyFont.svg#3478686034630b6bbb8f4a5751bfe071') format('svg'); /* Legacy iOS */
  font-style:   normal;
  font-weight:  400;
}

So option to pass keepQueryInUrl is added, which I left to be false by default, so nobody that relies on the current behaviour would suffer. css-loader and eventually webpack core down the road does not care about it, it just works. and both when relative (default) or absolute option is passed to resolve-url-loader.

    {
      test: /\.(scss)$/,
      loader: production
      ? ExtractTextPlugin.extract('css-loader!postcss-loader!resolve-url-loader?keepQueryInUrl!sass-loader?sourceMap')
      : 'style-loader!css-loader?sourceMap!postcss-loader!resolve-url-loader?keepQueryInUrl!sass-loader?sourceMap'
    }

Please revise. If all OK, please merge and bump npm package.

Regards, kroko

bholloway commented 8 years ago

Thanks for PR.

I'm about 1000km away from a computer right now. But expect something next week.

bholloway commented 8 years ago

Dear @kroko. In general I think this is pretty safe and I'm happy merge if it tests ok on your use case.

I value your PR so rather than writing myself I would want to merge your PR and give you credit for contributing.

However I have made some inline comments you might consider. If you have strong contrary opinion then just let me know. I will then test any final code on my code-base with the option turned off.

kroko commented 8 years ago

Sure, will look into your comments this weekend and change things according to them. Thanks!

bholloway commented 8 years ago

Ok looks good.

I will test backwards compatibility with a couple of projects. Can you just confirm the latest code works for your use case.

kroko commented 8 years ago

Latest code works with our use case.

bholloway commented 8 years ago

Ok @kroko thats published as 1.5.0.

Pleasure working with you. Let me know if there are any problems.