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

search does not stop at process.cwd() as intended, intension should change to support directories outside process.cwd() #8

Closed bholloway closed 8 years ago

bholloway commented 8 years ago

This issue comes out of #5 and the related ccorcos/webpack-sass-url-issue#2.

The original intention was to limit file search on encountering any of the following:

Currently there is a bug with the intension, as find-file.js#L102 allows paths such as ../. Meaning that if the search starts outside process.cwd() then it could include the entire filesystem.

However it is a valid use-case to allow operation outside of the project directory.

We need to determine a better limit than process.cwd() and ensure that its test function is not erroneous.

bholloway commented 8 years ago

@ccorcos please participate if you have any opinion on this.

ccorcos commented 8 years ago

Awesome. Could perhaps specify some kind of root.

bholloway commented 8 years ago

@ccorcos I've been looking at the resolve.root option that we talked about previously.

However I have at least one project where resolve.root is not meaningful. I'm inclined to decouple from this parameter and make the search limit some explicit setting.

I was thinking the loader expose a root parameter. I would feed this straight into path.resolve() so it would evaluate relative to process.cwd(). The default value would be . which would be consistent with the original intention.

One option is to make this a query parameter. For your use case I guess it would look like the following.

{ test: /\.scss$/, loader: "style!css!resolve-url?root=..!sass" }

Alternatively, we could make it a programatic configuration in which case it could be an arbitrary value. Once again your use case would look like the following.

{
  module: {
    loaders: [
      { test: /\.scss$/, loader: "style!css!resolve-url!sass" }
    ],
  },
  resolveUrlLoader: {
   root: path.resolve('..')
  }
}

Do you have any strong feeling on this?

ccorcos commented 8 years ago

looks good to me!

On Tue, Nov 3, 2015 at 3:00 PM, Ben Holloway notifications@github.com wrote:

@ccorcos https://github.com/ccorcos I've been looking at the resolve.root option that we talked about previously.

However I have at least one project where resolve.root is not meaningful. I'm inclined to decouple from this parameter and make the search limit some explicit setting.

I was thinking the loader expose a root parameter. I would feed this straight into path.resolve() so it would evaluate relative to process.cwd(). The default value would be . which would be consistent with the original intention.

One option is to make this a query parameter. For your use case I guess it would look like the following.

{ test: /.scss$/, loader: "style!css!resolve-url?root=..!sass" }

Alternatively, we could make it a programatic configuration https://webpack.github.io/docs/how-to-write-a-loader.html#programmable-objects-as-query-option in which case it could be an arbitrary value. Once again your use case would look like the following.

{ module: { loaders: [ { test: /.scss$/, loader: "style!css!resolve-url!sass" } ], }, resolveUrlLoader: { root: path.resolve('..') } }

Do you have any strong feeling on this?

— Reply to this email directly or view it on GitHub https://github.com/bholloway/resolve-url-loader/issues/8#issuecomment-153516136 .

bholloway commented 8 years ago

This is now live on npm as 1.4.0.

ccorcos commented 8 years ago

:+1: