fix(index): don't concat options.outputPath and options.publicPath
#246
Conversation
| @@ -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); | |||
There was a problem hiding this comment.
Just fix eslint problem.
| if (options.useRelativePath) { | ||
| const filePath = this.resourcePath; |
There was a problem hiding this comment.
filePath need only when we use options.useRelativePath
| "assets": Array [ | ||
| "output_path/9c87cbf3ba33126ffd25ae7f2f6bbafb.png9c87cbf3ba33126ffd25ae7f2f6bbafb.png", | ||
| ], | ||
| "source": "module.exports = __webpack_public_path__ + \\"9c87cbf3ba33126ffd25ae7f2f6bbafb.png\\";", |
There was a problem hiding this comment.
Add to tests assets and source to ensure outputPath don't concat with publicPath
michael-ciniawsky
changed the title
options.outputPath and options.publicPathoptions.outputPath and options.publicPath
|
@evilebottnawi Could this break setups currently 'relying' on this behavior in some way ? |
|
@michael-ciniawsky i will add tests to check this |
|
@michael-ciniawsky after investigate it will break build in some case. Should be do this in |
|
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 |
|
@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
added
status: Approved
semver: Patch
and removed
pr_action: Review
priority: 4 (important)
severity: 4 (inconvenient)
labels
|
Released in |
|
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. |
Can you give an example ? This sounds like different problem and might be a (accidentically) mistake made in this PR |
|
@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: So, instead of my file being output and served up with a URL of 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, Now, the emitted path ends up being like |
|
That's definitely a regression introduced on L47. Tracking issue is here (#249), thx for reporting and PR welcome and appreciated :). cc @evilebottnawi |
|
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. |
|
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: '/'
}
},``` |
|
@jacekk015 wait exactly failed? |
|
File not found only after update. Nothing else changed. |
|
@jacekk015 you should use |
|
@evilebottnawi Thanks! It worked! |
|
I had same problem as @jacekk015. Here is my diff to fix it from 1.1.6 to 1.1.8: Thank you for the changelog! |
|
Helpful! Thanks. |
- 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
Introduced in webpack-contrib/file-loader#246
) * 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
Fixes #160