Skip to content
This repository has been archived by the owner on Mar 17, 2021. It is now read-only.

revert(index): context takes precedence over issuer.context (options.useRelativePath) #260

Merged
merged 1 commit into from
Mar 1, 2018

Conversation

michael-ciniawsky
Copy link
Member

@michael-ciniawsky michael-ciniawsky added this to the 1.1.11 milestone Feb 27, 2018
@michael-ciniawsky michael-ciniawsky changed the title revert(index): context takes precedence over issuer.context (`opt… revert(index): context takes precedence over issuer.context (options.useRelativePath) Feb 27, 2018
Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@alexander-akait
Copy link
Member

@michael-ciniawsky need ping someone who have this bug and tests this branch

@zemadz
Copy link

zemadz commented Feb 28, 2018

I'm afraid it does not restore the behavior that was in 1.1.7. Tested with issue reproduction repository (https://github.com/zemadz/file-loader-relative-path-minimum-repro) and my actual project that uses file-loader for CSS generation.
I tried many combinations but were unable to achieve desired output with this branch @aea46b8b94b15e53e38761e0109850868c7a7b2d.

@michael-ciniawsky
Copy link
Member Author

@zemadz The following seems to work

{
  test: /\.(png)$/,
  use: [
    {
      loader: 'file-loader',
      options: {
        name: '[name].[ext]',
        // outputPath: paths.build,
        useRelativePath: true
      }
    }
  ]
}
assets/image.png
js/index.js
{
  test: /\.(png)$/,
  use: [
    {
      loader: 'file-loader',
      options: {
        name: '[name].[ext]',
        outputPath (url) {
          return 'test/' + url
        }
        useRelativePath: true
      }
    }
  ]
}
test/assets/image.png
js/index.js

Your're getting something like this with this patch applied atm right?

/Users/User/Github/file-loader-relative-path-minimum-repro/build/assets/image.png        
js/index.js

@zemadz
Copy link

zemadz commented Mar 1, 2018

With config:

{
  test: /\.(png)$/,
  use: [{
    loader: 'file-loader',
    options: {
      name: '[name].[ext]',
      // outputPath: paths.build,
      useRelativePath: true
    }
  }]
}

I get the expected files at build/assets/image.png and build/js/index.js, where the javascript exports module.exports = __webpack_require__.p + "assets/image.png";. I expect the export to equal module.exports = __webpack_require__.p + "../assets/image.png" since that would be the relative path to the image from the js file.

With the current pull request the following produces exactly the same output as the previous config:

{
  test: /\.(png)$/,
  use: [{
    loader: 'file-loader',
    options: {
      name: '[path][name].[ext]',
      // outputPath: paths.build,
      // useRelativePath: true
    }
  }]
}

@michael-ciniawsky
Copy link
Member Author

@zemadz The export points to the destination of the outputted file ? Can you explain the use case or how this currently breaks an application of yours ?

file loader

@zemadz
Copy link

zemadz commented Mar 1, 2018

I'm using Webpack for compiling SASS. The idea is that there is a scss directory for sources which will have matching outputs at css directory when compiled. Assets are not handled by Webpack bundle, but they can be referenced from the styles.

I've made an example with my use case (https://github.com/zemadz/file-loader-relative-path-css-repro). There are three branches master (1.1.8), working (1.1.7), revert (file-loader revert branch). I created a test that can be run with npm test to describe the expected output.

@michael-ciniawsky
Copy link
Member Author

This PR fixes the direct regression introduced in 3b071f5 @zemadz I will take checkout your second example soon, but due to extract-text-webpack-plugin && resolve-url-loader involved I'm pretty sure your particular issue lies elsewhere, there are known issues with this setup in regard to file path resolving

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants