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

Improved joiOptions object handling #6

Closed
andreesteve opened this issue Nov 21, 2017 · 4 comments
Closed

Improved joiOptions object handling #6

andreesteve opened this issue Nov 21, 2017 · 4 comments

Comments

@andreesteve
Copy link

Two suggestions:

  1. When a null or undefined value is passed as joiObject, just default it to empty object. That's what joi does. Currently the code throws exception if an object type is not passed.

  2. Invert the check for convert option.
    According to joi API docs, convert option defaults to true:

    convert - when true, attempts to cast values to the required types (e.g. a string to a number). Defaults to true.
    

    The exception would be that passing an empty object as joiOptions means that convert is enabled by default, thus the hook should replace the data with the converted value by default.

@eddyystop
Copy link
Collaborator

Thanks for posting.

I'm not sure what joiObject refers to. Neither this repo nor the joi docs seem to have such a name. If you mean joiSchema, I do want throw if its missing. Otherwise neither context.data nor context.result[.data] can be null, unless the hook is being used improperly. And I do want to throw in that case.

I thought about what you pointed out about the convert option. I feel, i.n general, its more important the hook should address its own environment rather than rigidly follow joi. Then I think its more a matter of taste. I wouldn't don't feel comfortable having values change on me, perhaps without me realizing it. I would rather explicitly state to do it.

There is also now the issue of this being breaking change.

@schettino
Copy link
Contributor

@eddyystop I agree with you, but once we decide to work with Joi, we make this decision being aware of what Joi provide us out of the box. I think it's more reliable to anyone who wants to validate with Joi, to keep its default options.

@eddyystop
Copy link
Collaborator

The repo has to remain backward compatible where possible. If you can do that, please submit a PR.

@marshallswain
Copy link
Member

Closing this now that #9 has been merged.

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

No branches or pull requests

4 participants