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

Nipple.request `options` are only optional if callback is omitted as well. #28

Closed
totherik opened this issue Aug 11, 2014 · 5 comments
Closed
Assignees
Labels
Milestone

Comments

@totherik
Copy link
Contributor

@totherik totherik commented Aug 11, 2014

The code and tests indicate that the options argument is intended to be optional, but it's technically only optional if the callback argument is omitted as well.

Two possibilities to correct this are:

  1. Update docs to reflect the existing behavior: request(method, uri, [options, [callback]])
  2. Change the behavior to make options truly optional. This is slightly complicated by the private param _trace, so the change might be more involved than just checking the number of arguments.
@hueniverse

This comment has been minimized.

Copy link
Member

@hueniverse hueniverse commented Aug 13, 2014

Option 1. We don't support dynamic APIs for runtime functions. This trips the v8 optimizer and kills performance. Omitting options in the middle of an arg chain is only something we allow in startup methods.

@geek

This comment has been minimized.

Copy link
Member

@geek geek commented Aug 13, 2014

Fixed by #30

@geek geek self-assigned this Aug 13, 2014
@geek geek closed this Aug 13, 2014
@totherik

This comment has been minimized.

Copy link
Contributor Author

@totherik totherik commented Aug 14, 2014

We don't support dynamic APIs for runtime functions.

Definitely agree with that approach. Wasn't sure sure how closely you follow that rule, but if that's the case it's possibly worth refactoring or restricting the read and the convenience method signatures.

@hueniverse

This comment has been minimized.

Copy link
Member

@hueniverse hueniverse commented Aug 14, 2014

@totherik can you open bugs for those?

@totherik

This comment has been minimized.

Copy link
Contributor Author

@totherik totherik commented Aug 14, 2014

For sure!

Done and done.

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