feat(middleware): add methods option (options.methods)

[ferdinando-ferreira]

What kind of change does this PR introduce?
Feature

Did you add tests for your changes?
Yes

Summary
This change adds a new option to the middleware: acceptedMethods, to allow for the middleware, optionally, to accept methods other than just GET.

Does this PR introduce a breaking change?
No, given that the new option is is optional and that its default value results in the same previously existing behaviour there won't be a breaking change.

Other information
The impact on the middleware itself is very limited, the purpose of this feature would be to allow for (let's say) webpack-serve, using the serve.devMiddleware option, to set this option and allow for the middleware to respond to methods other than only GET. Below is an excerpt of a webpack.config.js using this option along with a (soon to be proposed) similar option

  serve: {
  	hotClient: { host:  { client: '*', server: '0.0.0.0' } },
  	host: '0.0.0.0',
  	devMiddleware: {
  		publicPath: ProjectConfig.getOutput(target).publicPath,
  		acceptedMethods: ['GET', 'POST']
  	},
  	add: (app, middleware, options) => {
  		const historyOptions = {};
  		historyOptions.logger = console.log.bind(console);
  		historyOptions.acceptedMethods = ['GET', 'POST'];
  		app.use(convert(history(historyOptions)));
  	}
  }

The immediate effect of this change is that, when the action of a form inside webpack-serve tries to submit it to the application it is passed along by the middleware (to be treated by the application) instead of simply returning a 404.

[jsf-clabot]

All committers have signed the CLA.

[michael-ciniawsky]

I like the feature, but not happy with the naming (non-blocking)

[michael-ciniawsky]

options.methods or even options.methods.accept(ed) (leaving the possiblity to extend this option in the future) to be consistent with naming e.g options.headers ? (Ideally we cold group it under a options.http prop but that's sadly not possible atm)

[ferdinando-ferreira]

Ideally we cold group it under a options.http prop but that's sadly not possible atm

Is it a limitation caused by interaction with some other module? I ask that because it is possible, with an (otherwise) inconsequential change at

webpack-dev-middleware/lib/middleware.js

Line 27 in ebdceda

const acceptedMethods = context.options.acceptedMethods || ['GET'];

to make it

const acceptedMethods = (context.options.http ? context.options.http.methods : null) || ['GET'];

The webpack.config would then be

serve: {
  hotClient: { host:  { client: '*', server: '0.0.0.0' } },
  host: '0.0.0.0',
  devMiddleware: {
    publicPath: ProjectConfig.getOutput(target).publicPath,
    http: {
      methods: ['GET', 'POST']
    }
  },
  add: (app, middleware, options) => {
    const historyOptions = {};
    historyOptions.logger = console.log.bind(console);
    historyOptions.acceptedMethods = ['GET', 'POST'];
    app.use(convert(history(historyOptions)));
  }
},

I tested it in my environment and it works, what would be the impediment to use options.http.methods?

[michael-ciniawsky]

🤔 I tink this could work out and I'm personally in favor of making this change. It will likely involve some modifictaions to webpack-dev-server afterwards, but that's ok from my side (this will likely be the case anyways)

cc @evilebottnawi What do you think about adding it to options.http as options.http.mehods instead. Anything of concern with this approach ?

[michael-ciniawsky]

It's definitely more organized and we could eventually move options.headers under options.http (e.g options.http.headers) in the next major aswell then

[alexander-akait]

@michael-ciniawsky middleware usually do not hide options under http (example https://github.com/expressjs/cors#configuration-options), because they already control request/response in http context. I think methods is good name for option.

Also we can reorganize options (rename/move under other option) in next major. Here is all fine.

[michael-ciniawsky]

Yep, options.methods is equally fine aswell, we can eventually group them later in the next major, but it's a separate issue. So we all agree on moving forward with options.methods ? @ferdinando-ferreira ?

[ferdinando-ferreira]

Agreed! Made the change at the original PR (code, tests and docs) and tested in my environment. Seems good to go

[codecov]

Codecov Report

Merging #319 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #319      +/-   ##
==========================================
+ Coverage   96.81%   96.82%   +0.01%     
==========================================
  Files           7        7              
  Lines         251      252       +1     
==========================================
+ Hits          243      244       +1     
  Misses          8        8

Impacted Files	Coverage Δ
lib/middleware.js	94.23% <100%> (+0.11%)	⬆️

---

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 e0c5f11...091a0c0. Read the comment docs.

[alexander-akait]

/cc @michael-ciniawsky need release