fix: check existence of res.getHeader and set the correct Content-Type

[ulivz]

What kind of change does this PR introduce?

Bug fix

Did you add tests for your changes?

No tests needed.

Summary

This PR is tagged "bug fix" but actually a feature request.

I'm the maintainer of VuePress and recently we found a very influential issue(vuepress dev: throws res.getHeader() is not a function) but we cannot fix it by ourself. and it's introduced by b2a6fed. (But after careful investigation, I found that this is the legacy of history.)

This is due to VuePress@0.x depends on webpack-serve@1.0.2, webpack-serve depends on koa-webpack, and koa-webpack doesn't lock the version of webpack-dev-middleware:

https://github.com/shellscape/koa-webpack/blob/0ea07dbd11f899f0e419808f2472b2d832d0d9c3/package.json#L24

"webpack-dev-middleware": "^3.0.0",

The issue is already fixed in shellscape/koa-webpack#110 (released as v5.2.2) but we cannot enjoy this fix since webpack-serve@1.0.2 depends on koa-webpack@4.x.

One way to resolve the issue is to update webpack-serve dependency to 2.x.
However, webpack-serve introduced breaking changes in v2 and we‘re not going to make any technological changes for VuePress@0.x because it's stable enough and we're also focused on 1.x.

Since webpack-serve has been deprecated, the best way I can think of is to check the existence of res.getHeader at webpack-dev-middleware. Of course, all of this is because the author of koa-webpack did not pass in the getHeader method in 4.x at that time:

https://github.com/shellscape/koa-webpack/blob/v4.0.0/index.js#L48

  return (context, next) => Promise.all([
    waitMiddleware(),
    new Promise((resolve) => {
      dev(context.req, {
        end: (content) => {
          context.body = content; // eslint-disable-line no-param-reassign
          resolve();
        },
        setHeader: context.set.bind(context),
        locals: context.state
      }, () => resolve(next()));
    })
  ]);

I'll appreciate you if you accept this change, Thanks!

Does this PR introduce a breaking change?

No.

Other information

[jsf-clabot]

All committers have signed the CLA.

[codecov]

Codecov Report

Merging #385 into master will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master     #385   +/-   ##
=======================================
  Coverage   96.96%   96.96%           
=======================================
  Files           7        7           
  Lines         264      264           
=======================================
  Hits          256      256           
  Misses          8        8

Impacted Files	Coverage Δ
lib/middleware.js	94.54% <100%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dba5e02...2337b4b. Read the comment docs.

[codecov]

Codecov Report

Merging #385 into master will increase coverage by 0.08%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #385      +/-   ##
==========================================
+ Coverage   96.77%   96.86%   +0.08%     
==========================================
  Files          12       13       +1     
  Lines         279      287       +8     
  Branches       81       83       +2     
==========================================
+ Hits          270      278       +8     
  Misses          9        9

Impacted Files	Coverage Δ
test/mock-express/index.js	100% <100%> (ø)
lib/middleware.js	94.64% <100%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eb2e32b...dca42ae. Read the comment docs.

[hiroppy]

In any case, if we don't have res.getHeader will be an error here so I think this change is no problem.

[alexander-akait]

We don't support webpack-serve, it is deprecated and officially doesn't supporting, and we need tests

[alexander-akait]

I strongly recommend migrate from webpack-serve to avoid this problem in future, also it is doesn't works with webpack@5, so you still need migrate on other solution than webpack-serve

[ulivz]

@evilebottnawi But this issue is truly introduced by this package.

Why not responsible for your upstream if you regard your package as the cornerstone?

[alexander-akait]

@ulivz upstream? As i said above webpack-serve is deprecated, but we can accept PR, need tests for this to avoid regression in future

[ulivz]

Did you mean that I should write a test for KOA usage as https://github.com/webpack/webpack-dev-middleware/blob/master/test/tests/server.js

[alexander-akait]

@ulivz no, just defined (mock) getHeader method as undefined and add test to ensure all works fine

[ulivz]

@evilebottnawi Got it!

[ulivz]

I tried to add test under test/tests/server.js, but unfortunately I found that I cannot cover the res.getHeader method under express since express depends on it under the hood.

ref: https://github.com/expressjs/express/blob/master/lib/response.js#L784

res.getHeader comes from http.ServerResponse.prototype since res in express inherits from it but KOA didn't do it.

So shouldn't I write a test for KOA usage?

Here is my test case:

  describe.only('custom Content-Type when res.setHeader is undefined', () => {
    before((done) => {
      app = express();
      app.use((req, res, next) => {
        // Cover getHeader at the prototype.
        res.getHeader = undefined;
        res.set('Content-Type', 'application/octet-stream');
        next();
      });

      const compiler = webpack(webpackConfig);
      instance = middleware(compiler, {
        stats: 'errors-only',
        logLevel
      });
      app.use((req, res, next) => {
        // Recover getHeader;
        delete res.getHeader;
        next();
      });
      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);
    });
  });

[alexander-akait]

Just mock value, no need import

[ulivz]

Writing a new test or still in test/tests/server.js?

[alexander-akait]

@ulivz server.js

[ulivz]

Sorry for my poor intimate knowledge of sinon，I just read the docs of sinon, it told me:

Mocks (and mock expectations) are fake methods (like spies) with pre-programmed behavior (like stubs) as well as pre-programmed expectations.

But for now we need to mock a undefined, so how should I mock the getHeader and ensure the test work?

Or, do you mean that I should mock the express?

[alexander-akait]

Yes, just mock express where getHeader is undefined

[ulivz]

@evilebottnawi Tests updated and passed, could you help re-review this MR? Thanks.

This out of PR is very important to us. Thanks!

[ulivz]

Any progress here？

[alexander-akait]

@ulivz i was on vacation

[ulivz]

Thanks~