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

Doesn't work with file path format of sass-loader #3

Closed shanear closed 8 years ago

shanear commented 8 years ago

The sass-loader seems to format the source map file paths sources as absolute file paths without preceding slashes. The resolve-url-loader expects all absolute file paths to have a preceeding slash. This is causing all generated URLs to be bad.

Is anyone else experiencing this?

Example:

example file path in the sources: this/is/an/absolute/path.scss

example source file path generated: /base/path/of/the/project/this/is/an/absolute/path.scss

shanear commented 8 years ago

this commit is a short term fix for me, but I'm not sure what the proper overall fix should be.

https://github.com/shanear/resolve-url-loader/commit/cdec26794a30ffa4a3a3c771e04032c1b53476b8

bholloway commented 8 years ago

Understood. Will get right on it.

bholloway commented 8 years ago

Hey @shanear. Is the original sass import an absolute path?

shanear commented 8 years ago

@bholloway it's a module path, for a node module. I'm not sure if this gets converted to an absolute path.

@import "~styleguide/main";

bholloway commented 8 years ago

@shanear Ok I have a similar setup.

My source-map sources are relative to the js from which I require() the .scss - I presume this is the SASS root. I'm not sure why yours are absolute, but we need to be robust for either case.

This code in question reused from SASS outside webpack and inherited a coupe of fixes that may not be required:

I will retain those fixes as defensive programming, at least until I can comprehensively discount them. The following should be more robust for all cases:

  function absolutePath(value, i, array) {
    var location = value
      .replace(/\b[\\\/]+\b/g, path.sep) // remove duplicate slashes (windows)
      .replace(/^[\\\/]\./, '.');        // remove erroneous leading slash on relative paths
    array[i] = path.resolve(filePath, location);
  }

I've just noticed some bigger problems with source maps. If you can give me a couple of hours I should have a branch today with some comprehensive fixes that you can try.

bholloway commented 8 years ago

@shanear I have a branch that you can try:

npm i bholloway/resolve-url-loader#bugfix/source-maps

I have tested with a small todo-mvc project. I will try with a larger project and wait for your feedback.

shanear commented 8 years ago

I tried your branch, and it's still not working.

But in trying it, i've realized that everything works with the webpack build command. It only fails when using webpack-dev-server.

This is because the sourceMap file path is different between those two commands.

1) webpack-dev-server will return a path that looks like: this/is/an/absolute/path/main.scss and 2) webpack will return the same path as relative, like: ../../path/main.scss

My temporary hack fixes 1) but breaks 2). Both master and bugfix/source-maps work for 2) but break on 1).

I'm not sure if this is an issue with resolve-url-loader, or sass-loader, or webpack-dev-server. Will keep digging. Thanks a ton for your help so far!

bholloway commented 8 years ago

Ok I'll try that too

shanear commented 8 years ago

So after a lot more digging, the reason these pseudo-absolute urls are being generated in webpack-dev-server is because it's blowing out the output.path option to root for some reason: https://github.com/webpack/webpack-dev-server/blob/master/bin/webpack-dev-server.js#L75

Then the sass-loader will use the output.path option to create the sources for the sourcePath: https://github.com/jtangelder/sass-loader/blob/master/index.js#L262

By the time it gets to poor ole' resolve-url-loader the path is relative to the root and not the expected output path. I'm not sure the right solution (and i'm thinking it might not be in resolve-url-loader), just wanted to update you on what I found.

Thoughts?

bholloway commented 8 years ago

Thanks @shanear for your digging. I was able to reproduce with webpack-dev-server.

I have added some more defensive coding. It should work for you now unless there is something else at play. Take a look at the code and let me know if it is robust enough.

So is it your conclusion that it is an error in webpack-dev-server? I was not aware of this tool until just now (I use the browsersync plugin myself).

shanear commented 8 years ago

Sweet that works!

That's my best guess. Another possibility is that it's an error in sass-loader to use the output.path option to generate the source maps. I'll see what they have to say.

bholloway commented 8 years ago

Published 1.2.0.

Thanks for all your patience and help diagnosing this one!

So ultimately index.js:98 is defensive coding to compensate for the external issue webpack/webpack-dev-server#266.