Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Do not overwrite Content-Type #377

Merged
merged 1 commit into from
Mar 6, 2019
Merged

Do not overwrite Content-Type #377

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 3 additions & 1 deletion lib/middleware.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,9 @@ module.exports = function wrapper(context) {
contentType += '; charset=UTF-8';
}

res.setHeader('Content-Type', contentType);
if (!res.getHeader('Content-Type')) {
res.setHeader('Content-Type', contentType);
}
res.setHeader('Content-Length', content.length);
Copy link
Member

Choose a reason for hiding this comment

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

What do you think should we add check here too? I think in theory it is also possible to set own Content-Length, don't known real use case, but still think what we should add check here too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was my initial thought too. But I think matters are more complicated... It looks like ExpressJS itself sets the Content-Length

https://github.com/expressjs/express/blob/3d10279826f59bf68e28995ce423f7bc4d2f11cf/lib/response.js#L194

In fact line 85 seems to be not functional at all.

Copy link
Member

Choose a reason for hiding this comment

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

@dmohns good catch, can you create issue? Will be great if you help to use investigate this, let's merge this as is

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 Will do.

Copy link

Choose a reason for hiding this comment

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

hello，this commit make error，"TypeError: res.getHeader is not a function"，my node version is v10.15.2，how can i solve this problem

Copy link

Choose a reason for hiding this comment

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

👍 Will do.

hello，this commit make error，"TypeError: res.getHeader is not a function"，my node version is v10.15.2，how can i solve this problem


const { headers } = context.options;
Expand Down
21 changes: 21 additions & 0 deletions test/tests/server.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -259,6 +259,27 @@ describe('Server', () => {
});
});

describe('custom Content-Type', () => {
before((done) => {
app = express();
const compiler = webpack(webpackConfig);
instance = middleware(compiler, {
stats: 'errors-only',
logLevel,
headers: { 'Content-Type': 'application/octet-stream' }
});
app.use(instance);
listen = listenShorthand(done);
});
after(close);

it('Do not guess mime type if Content-Type header is found ', (done) => {
request(app).get('/bundle.js')
.expect('Content-Type', 'application/octet-stream')
.expect(200, done);
});
});

describe('custom mimeTypes', () => {
before((done) => {
app = express();
Expand Down