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.

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

feat(rest): allow assigning express settings #2423

Merged
merged 1 commit into from Feb 27, 2019

Conversation

Projects
None yet
4 participants
@ludohenin
Copy link
Contributor

ludohenin commented Feb 18, 2019

Add a new rest configuration for queryParser

Fixes #2420

Checklist

  • npm test passes on your machine
  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide
  • API Documentation in code was updated
  • Documentation in /docs/site was updated
  • Affected artifact templates in packages/cli were updated
  • Affected example projects in examples/* were updated

@ludohenin ludohenin requested review from bajtos and raymondfeng as code owners Feb 18, 2019

@ludohenin

This comment has been minimized.

Copy link
Contributor Author

ludohenin commented Feb 18, 2019

@raymondfeng not sure what to test here. I've made this PR to start the discussion.

@ludohenin ludohenin force-pushed the ludohenin:feat/rest/qs-options branch from 9ee990b to 021262f Feb 18, 2019

Object.entries(this.config.expressSettings || {}).forEach(
([key, value]) => {
if (typeof value === 'function') {
value = value.bind(this);

This comment has been minimized.

@raymondfeng

raymondfeng Feb 18, 2019

Member

Why bind here? I think it's the responsibility of the code that provides the configuration to supply the correct function.

This comment has been minimized.

@ludohenin

ludohenin Feb 19, 2019

Author Contributor

not sure, I was thinking it's better than having express bound to this. Removed it.

This comment has been minimized.

@raymondfeng

raymondfeng Feb 20, 2019

Member

BTW, we need to keep the default 'query parser': 'extended' if not present. I fixed it in my clone (to be pushed back to the PR)

Show resolved Hide resolved docs/site/Server.md Outdated
@raymondfeng

This comment has been minimized.

Copy link
Member

raymondfeng commented Feb 18, 2019

To unit test the expressSettings, you can create a subclass of RestServer, which can then access _expressApp and use _expressApp.get(...) to verify.

@ludohenin ludohenin force-pushed the ludohenin:feat/rest/qs-options branch from 021262f to ac65294 Feb 19, 2019

@ludohenin

This comment has been minimized.

Copy link
Contributor Author

ludohenin commented Feb 19, 2019

@raymondfeng should be closer now

@raymondfeng

This comment has been minimized.

Copy link
Member

raymondfeng commented Feb 19, 2019

@ludohenin Please run npm run lint:fix to fix style/formatting issues reported by CI.

@ludohenin ludohenin force-pushed the ludohenin:feat/rest/qs-options branch from ac65294 to c7ae470 Feb 19, 2019

@ludohenin

This comment has been minimized.

Copy link
Contributor Author

ludohenin commented Feb 19, 2019

@raymondfeng done but there's still an error with node 10

@ludohenin ludohenin changed the title feat(rest): allow query parser configuration feat(rest): allow assigning express settings Feb 19, 2019

@raymondfeng

This comment has been minimized.

Copy link
Member

raymondfeng commented Feb 19, 2019

@ludohenin Thanks. The CI failure is not related to your change.

@raymondfeng

This comment has been minimized.

Copy link
Member

raymondfeng commented Feb 20, 2019

@ludohenin I rebased your branch against latest master. Can you grant me the permission per https://help.github.com/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork/ so that I can push such changes to the PR?

@ludohenin

This comment has been minimized.

Copy link
Contributor Author

ludohenin commented Feb 21, 2019

@raymondfeng should be good now

@raymondfeng raymondfeng force-pushed the ludohenin:feat/rest/qs-options branch 3 times, most recently from 2fab989 to f44bc95 Feb 21, 2019

@raymondfeng raymondfeng force-pushed the ludohenin:feat/rest/qs-options branch from f44bc95 to b3c8b8f Feb 25, 2019

@raymondfeng raymondfeng force-pushed the ludohenin:feat/rest/qs-options branch from b3c8b8f to 3309117 Feb 25, 2019

@raymondfeng raymondfeng merged commit 962f1cb into strongloop:master Feb 27, 2019

4 checks passed

clahub All contributors have signed the Contributor License Agreement.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.005%) to 90.642%
Details
@raymondfeng

This comment has been minimized.

Copy link
Member

raymondfeng commented Feb 27, 2019

@ludohenin PR landed. Thanks!

@ludohenin ludohenin deleted the ludohenin:feat/rest/qs-options branch Feb 28, 2019

@@ -214,7 +214,7 @@ export class RestServer extends Context implements Server, HttpServerLike {
protected _setupRequestHandlerIfNeeded() {
if (this._expressApp) return;
this._expressApp = express();
this._expressApp.set('query parser', 'extended');
this._applyExpressSettings();

This comment has been minimized.

@bajtos

bajtos Feb 28, 2019

Member

If I am reading these lines correctly, then:

  • before the change, we were explicitly configuring the Express instance to use extended query parser
  • now we use the default query parser configuration as provided by Express

I see that Express uses extended query parser by default, so I guess this is ok.

https://expressjs.com/en/4x/api.html#app.settings.table

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.