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

support passing most qs options to restify.queryParser #1209

Merged
merged 2 commits into from Oct 26, 2016

Conversation

trentm
Copy link
Contributor

@trentm trentm commented Oct 25, 2016

Fixes #925

@trentm
Copy link
Contributor Author

trentm commented Oct 25, 2016

@yunong @DonutEspresso poke. Thanks.

My intention is to:

  • get this in and publish restify@4.2.0
  • submit a PR to restify-plugins to allow passing the same options to queryParser there.

Yunong, if you recall, you and I discussed this work ages ago in private chat.

@trentm
Copy link
Contributor Author

trentm commented Oct 25, 2016

Oh, also, I'd like to do the 'make cutarelease' Makefile task for node-restify.git as I did for restify/clients.git (restify/clients@d691925)

plainObjects: true,
allowDots: true
};
Object.keys(EXPOSED_QS_OPTIONS).forEach(function (k) {
Copy link
Member

Choose a reason for hiding this comment

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

Not sure about the performance impact of Object.keys, but presumably you could use https://www.npmjs.com/package/lodash.foreach to optimize this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

restify 4.x doesn't have a dep on any lodash modules. I don't think adding a dep is justified for this here. If iteration code is in a hot path I typically use for (var i = 0; i < arr.length; i++) ... which is typically much faster. However, this isn't a hot path at all: it is in section of queryParser that is just called once at server or endpoint setup. It isn't called for every request.

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough :)

allowDots: true
};
Object.keys(EXPOSED_QS_OPTIONS).forEach(function (k) {
EXPOSED_QS_OPTIONS[k](options[k], k);
Copy link
Member

Choose a reason for hiding this comment

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

Mind dropping a comment to describe what's happening here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@yunong
Copy link
Member

yunong commented Oct 26, 2016

LGTM, feel free to merge.

@trentm trentm merged commit 15c9bf3 into restify:4.x Oct 26, 2016
@trentm
Copy link
Contributor Author

trentm commented Oct 26, 2016

Thanks!

@trentm trentm deleted the 4.x-queryparser-options branch October 26, 2016 20:48
@trentm
Copy link
Contributor Author

trentm commented Oct 26, 2016

restify@4.2.0 published with this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants