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

fix(index): don't concat options.outputPath and options.publicPath #246

Merged
merged 1 commit into from
Feb 19, 2018

Conversation

alexander-akait
Copy link
Member

Fixes #160

@@ -10,7 +10,7 @@ export default function loader(content) {

validateOptions(schema, options, 'File Loader');

const context = options.context || this.rootContext || this.options && this.options.context
const context = options.context || this.rootContext || (this.options && this.options.context);
Copy link
Member Author

Choose a reason for hiding this comment

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

Just fix eslint problem.

if (options.useRelativePath) {
const filePath = this.resourcePath;
Copy link
Member Author

Choose a reason for hiding this comment

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

filePath need only when we use options.useRelativePath

"assets": Array [
"output_path/9c87cbf3ba33126ffd25ae7f2f6bbafb.png9c87cbf3ba33126ffd25ae7f2f6bbafb.png",
],
"source": "module.exports = __webpack_public_path__ + \\"9c87cbf3ba33126ffd25ae7f2f6bbafb.png\\";",
Copy link
Member Author

Choose a reason for hiding this comment

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

Add to tests assets and source to ensure outputPath don't concat with publicPath

@michael-ciniawsky michael-ciniawsky changed the title fix: don't concat options.outputPath and options.publicPath fix(index): don't concat options.outputPath and options.publicPath Feb 14, 2018
@michael-ciniawsky michael-ciniawsky added this to the 1.1.6 milestone Feb 14, 2018
@michael-ciniawsky
Copy link
Member

@evilebottnawi Could this break setups currently 'relying' on this behavior in some way ?

@alexander-akait
Copy link
Member Author

@michael-ciniawsky i will add tests to check this

@alexander-akait
Copy link
Member Author

@michael-ciniawsky after investigate it will break build in some case. Should be do this in @next?

@alexander-akait alexander-akait changed the base branch from master to next February 19, 2018 09:49
@alexander-akait alexander-akait changed the base branch from next to master February 19, 2018 09:50
@michael-ciniawsky
Copy link
Member

Can you give an example of a breaking scenario ? Yes it likely can break some setups, but it's still a bug nevertheless. Bit gray area here 😛 If avoidable a patch would be better...

@alexander-akait
Copy link
Member Author

@michael-ciniawsky many people remove public path from output with difference approaches, It's hard to say who does what exactly breaks down, but i agree this is bug, i can take care about new issue after merge this in master 👍

@michael-ciniawsky
Copy link
Member

Released in v1.1.7 🎉

@michael-ciniawsky michael-ciniawsky removed this from the 1.1.8 milestone Feb 19, 2018
@Android3000
Copy link

Just an FYI, it might be nice to label this as a potentially breaking change in the release notes, as it looks like you were concerned about it, and I can vouch for this almost immediately causing issues with our build. We were definitely misunderstanding how outputPath was working when using it as a function, and can see by the intent now that it probably wasn't the "right way" and may need to just change our direction.

Prior to this change the URL was being set to the result of the outputPath function result. With this change, the URL is now getting appended to the end of the outputPath function result. This caused issues with our asset output and failures to load them.

@michael-ciniawsky
Copy link
Member

Prior to this change the URL was being set to the result of the outputPath function result. With this change, the URL is now getting appended to the end of the outputPath function result. This caused issues with our asset output and failures to load them.

Can you give an example ? This sounds like different problem and might be a (accidentically) mistake made in this PR

@Android3000
Copy link

Android3000 commented Feb 19, 2018

@michael-ciniawsky Sure -- again, this is probably just a side effect of us (ab)using the outputPath function capability incorrectly.

Prior to this release, providing an outputPath function would see an argument passed in to it of the current URL of the file. We were using a name like: "[path][name]-[hash].[ext]", and we'd see an argument like "client/fonts/font-icons/icons-123456789.svg" given to the function. The return value of that outputPath function would end up being where the file would be emitted -- the function basically acted like a way to transform the file path. So, for instance, I would use it to just organize some things, e.g.
if (url.startsWith("client/") return url.replace("client/", "assets/");

So, instead of my file being output and served up with a URL of "site.com/client/fonts/font-icons/icons-123456789.svg", it would instead be "site.com/assets/fonts/font-icons/icons-123456789.svg". I was using this to basically help organize the output of some assets for in-house vs. vendor vs. other based on the path prefix I was getting.

With this latest change however (due to the removed "else if" conditional on line 47 of the previous version of the file), the URL gets appended to the result of the output path function (see the change on line 47 of the current version, outputPath += url). Before, in the "else if" that would handle outputPath being present, the url itself was being replaced by the result of the output path (previous version line 51, url = outputPath).

Now, the emitted path ends up being like "assets/fonts/font-icons/icons-123456789.svgclient/fonts/font-icons/icons-123456789.svg", and the re-written URL ends up trying to serve it up at "site.com/client/fonts/font-icons/icons-123456789.svg".

@michael-ciniawsky
Copy link
Member

That's definitely a regression introduced on L47. Tracking issue is here (#249), thx for reporting and PR welcome and appreciated :). cc @evilebottnawi

@chtseac
Copy link

chtseac commented Feb 20, 2018

Bugfix or not, this is a behaviour change in that it changes the public paths of the generated files. It should have increased the major by 1.

@jacekk015
Copy link

jacekk015 commented Feb 20, 2018

After update webpack-dev-server failed to serve my font files.

{
      test: /\.(eot|woff|woff2|ttf|svg)$/,
      loader: 'file-loader',
      options: {
        name: '[name].[ext]',
        outputPath: 'css/fonts/',
        publicPath: '/'
      }
    },```

@alexander-akait
Copy link
Member Author

@jacekk015 wait exactly failed?

@jacekk015
Copy link

file_loader

File not found only after update. Nothing else changed.

@alexander-akait
Copy link
Member Author

@jacekk015 you should use publicPath: 'css/fonts/'

@jacekk015
Copy link

@evilebottnawi Thanks! It worked!

@olore
Copy link

olore commented Feb 21, 2018

I had same problem as @jacekk015. Here is my diff to fix it from 1.1.6 to 1.1.8:

-            'file-loader?name=[name]-[hash].[ext]&publicPath=../../../&outputPath=./assets/'
+            'file-loader?name=[name]-[hash].[ext]&publicPath=../../../assets/&outputPath=./assets/'

Thank you for the changelog!

@vimalans
Copy link

Helpful! Thanks.

agilgur5 added a commit to agilgur5/front-end-base that referenced this pull request Aug 10, 2018
- webpack-serve is the more modern version of webpack-dev-server
  - made by some of the same folks I believe

- also add webpack-serve-waitpage for build progress and
  webpack-serve-overlay for error overlays
  - overlay needed a new entry so that it show errors that occur
    after it's conditionally `require`d

- debug a bunch of publicPath and routing issues and add more explicit
  comments
  - see webpack-contrib/file-loader#246 ,
    webpack/webpack-dev-middleware#269 , and
    webpack-contrib/webpack-serve#238
  - TODO: experiment with publicPath for production / CDN as it may
    not work perfectly anymore

- (docs): add docs for hmr and reword some of README
Gerhut added a commit to microsoft/pai that referenced this pull request Sep 18, 2018
Gerhut added a commit to microsoft/pai that referenced this pull request Oct 11, 2018
)

* Webportal: Use tilde-version to apply the patches of each deps

* Webportal: fix public path

Introduced in webpack-contrib/file-loader#246

* Use yarn.lock instead of package-lock.json

* REST server: Use tilde-version to apply the patches of each deps

* Change command of dependencies installing
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.

7 participants