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

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

Merged
merged 7 commits into from
Aug 21, 2018
Merged

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ebdceda
Add the option `acceptedMethods`
ferdinando-ferreira Aug 20, 2018
60aa130
Update README.md
ferdinando-ferreira Aug 20, 2018
795b08a
Tests for the option `acceptedMethods`
ferdinando-ferreira Aug 20, 2018
df4654f
Tests for the option `acceptedMethods` : smallfix, line ending
ferdinando-ferreira Aug 20, 2018
aa06f77
Changed option name from acceptedMethods to methods
ferdinando-ferreira Aug 21, 2018
b195846
Update README.md
ferdinando-ferreira Aug 21, 2018
091a0c0
Changed option name from acceptedMethods to methods
ferdinando-ferreira Aug 21, 2018
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
7 changes: 7 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,13 @@ for the Object.

_Note: The `publicPath` property is required, whereas all other options are optional_

### acceptedMethods
Copy link
Member

@michael-ciniawsky michael-ciniawsky Aug 20, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

@ferdinando-ferreira ferdinando-ferreira Aug 20, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Member

@michael-ciniawsky michael-ciniawsky Aug 20, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

🤔 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 ?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

@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.

Copy link
Member

Choose a reason for hiding this comment

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

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 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


Type: `Array`
Default: `[ 'GET' ]`

This property allows a user to pass the list of HTTP request methods accepted by the server.

### headers

Type: `Object`
Expand Down
3 changes: 2 additions & 1 deletion lib/middleware.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,8 @@ module.exports = function wrapper(context) {
}));
}

if (req.method !== 'GET') {
const acceptedMethods = context.options.acceptedMethods || ['GET'];
if (acceptedMethods.indexOf(req.method) === -1) {
return goNext();
}

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

describe('accepted methods', () => {
before((done) => {
app = express();
const compiler = webpack(webpackConfig);
instance = middleware(compiler, {
stats: 'errors-only',
acceptedMethods: ['POST'],
logLevel,
publicPath: '/public/'
});
app.use(instance);
listen = listenShorthand(done);
});
after(close);

it('POST request to bundle file with acceptedMethods set to [\'POST\']', (done) => {
request(app).post('/public/bundle.js')
.expect('Content-Type', 'application/javascript; charset=UTF-8')
.expect('Content-Length', '3645')
.expect(200, /console\.log\('Hey\.'\)/, done);
});

it('GET request to bundle file with acceptedMethods set to [\'POST\']', (done) => {
request(app).get('/public/bundle.js')
.expect(404, done);
});
});

describe('no index mode', () => {
before((done) => {
app = express();
Expand Down