Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
fix: lack of trailing slash on publicPath string option
  • Loading branch information
karlvr committed Apr 5, 2019
commit fab158dbcbd1d477846fca2fb3a5ff44d88b0731
4 changes: 3 additions & 1 deletion src/loader.js
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,9 @@ export function pitch(request) {
const childFilename = '*'; // eslint-disable-line no-path-concat
const publicPath =
typeof query.publicPath === 'string'
? query.publicPath
? query.publicPath.endsWith('/')
? query.publicPath
: `${query.publicPath}/`
: typeof query.publicPath === 'function'
? query.publicPath(this.resourcePath, this.rootContext)
Copy link
Member

Choose a reason for hiding this comment

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

We don't need here url as in file-loader because file-loader for other purpose (answer on above question)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@evilebottnawi in file-loader it's because file-loader can choose to output the file itself? Doesn't the plugin do exactly that?

Copy link
Contributor

Choose a reason for hiding this comment

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

@karlvr @evilebottnawi Just wanted to let you know that this introduces a breaking change in the case when the publicPath is set to an empty string meaning that resources are located in the same directory as the css file, e.g. dist/app.css, dist/aResource.png. With the introduced change the publicPath is transformed to a / and so the resource path becomes /aResource.png which is clearly not desired.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@evilebottnawi @rmja

That change was to follow the same behaviour in https://github.com/webpack-contrib/file-loader/blob/master/src/index.js#L37

I'm really sorry about this regression! @evilebottnawi shall we introduce a special case for an empty string here?

Copy link
Contributor

Choose a reason for hiding this comment

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

@karlvr I have created #384

: this._compilation.outputOptions.publicPath;
Expand Down
2 changes: 2 additions & 0 deletions test/cases/publicpath-trailing-slash/expected/main.css
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
body { background: red; background-image: url(/static/img/cd0bb358c45b584743d8ce4991777c42.svg); }

1 change: 1 addition & 0 deletions test/cases/publicpath-trailing-slash/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
import './style.css';
1 change: 1 addition & 0 deletions test/cases/publicpath-trailing-slash/react.svg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
1 change: 1 addition & 0 deletions test/cases/publicpath-trailing-slash/style.css
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
body { background: red; background-image: url(./react.svg); }
34 changes: 34 additions & 0 deletions test/cases/publicpath-trailing-slash/webpack.config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
const Self = require('../../../');

module.exports = {
entry: './index.js',
module: {
rules: [
{
test: /\.css$/,
use: [
{
loader: Self.loader,
options: {
publicPath: '/static/img'
}
},
'css-loader',
],
}, {
test: /\.(svg|png)$/,
use: [{
loader: 'file-loader',
options: {
filename: '[name].[ext]'
}
}]
}
],
},
plugins: [
new Self({
filename: '[name].css',
}),
],
};