docs: ✏️ improve docs for server.rules()#4141
docs: ✏️ improve docs for server.rules()#4141devinivy merged 2 commits intohapijs:masterfrom damusix:master
Conversation
server.rules() was missing a good example and explanation was a bit obscure.
devinivy
left a comment
devinivy
left a comment
There was a problem hiding this comment.
Thanks a lot this contribution! I made some notes in here, mostly based around making the example more self-contained. If you're alright with my proposed changes I think this would be good to go just by accepting those, but there are a few additional comments if you're interested and have the time to dig into them.
API.md
Outdated
| realm. This means the processor defined by the plugin override the config generated by the root | ||
| processor if they overlap. The route `config` overrides the rules config if the overlap. | ||
| realm. This means the processor defined by the plugin overrides the config generated by the root | ||
| processor if they overlap. `route.config` overrides `route.rules` if the resulting processed config overlaps. |
There was a problem hiding this comment.
I searched for other places where "route.config" is referenced in the docs and didn't find anything, so for consistency I just rephrased this slightly. For some reason I think it's also nice to start a sentence with something other than code.
| processor if they overlap. `route.config` overrides `route.rules` if the resulting processed config overlaps. | |
| processor if they overlap. Similarly, the route's own config overrides the config produced by the rules processors. |
| if (!rules) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Thanks for illustrating this detail 👍
There was a problem hiding this comment.
This is redundant, as it should never trigger for the example. Otherwise hapi has botched the validateSchema validation of the rules object.
There was a problem hiding this comment.
@kanongil perhaps this entire block could be wrapped in an if block https://github.com/hapijs/hapi/blob/master/lib/route.js#L67
...
const rules = route.rules || config.rules;
let rulesConfig = {};
if (rules) {
const rulesConfig = internals.rules(rules, { method, path, vhost }, server);
}
...
API.md
Outdated
| } | ||
|
|
||
| if (rules.myCustomPre) { | ||
|
|
API.md
Outdated
| } | ||
|
|
||
| if (rules.payload) { | ||
| options.validate.payload = Joi.object(rules.payload); |
There was a problem hiding this comment.
This would fail since options.validate doesn't exist.
There was a problem hiding this comment.
| options.validate.payload = Joi.object(rules.payload); | |
| options.validate = { payload: Joi.object(rules.payload) }; |
| if (rules.myCustomPre) { | ||
|
|
||
| options.pre = [ | ||
| myPreHelper(...rules.myCustomPre) |
There was a problem hiding this comment.
Would it be possible to contrive a simple myPreHelper so that this example can be more self-contained?
API.md
Outdated
| validate: { | ||
| entity: 'user' | ||
| } | ||
| } |
API.md
Outdated
| server.rules(processor, realmRules); | ||
|
|
||
| server.route({ | ||
| ..., |
There was a problem hiding this comment.
It would be great to replace this placeholder with some method and path to make this example more complete.
There was a problem hiding this comment.
| ..., | |
| method: 'GET', | |
| path: '/', |
API.md
Outdated
| auth: Joi.string(), | ||
| myCustomPre: Joi.array().min(2).items(Joi.string()), | ||
| payload: Joi.object() | ||
| } |
API.md
Outdated
| const realmRules = { | ||
| validate: { schema: validateSchema } | ||
| }; | ||
|
|
||
| server.rules(processor, realmRules); |
There was a problem hiding this comment.
The goal here is just to make things more concise and avoid the "realms" reference for those unfamiliar with that hapi concept.
| const realmRules = { | |
| validate: { schema: validateSchema } | |
| }; | |
| server.rules(processor, realmRules); | |
| server.rules(processor, { | |
| validate: { schema: validateSchema } | |
| }); |
There was a problem hiding this comment.
I will update this when I get a chance some time this week. Thank you for the feedback!
|
@devinivy changes have been made! |
devinivy
left a comment
devinivy
left a comment
There was a problem hiding this comment.
There's chatter in slack about rules() and there may be some followup to do here, but for now this is great and is a very useful contribution. Thanks!
* update handlebars dependency version handlebars is only a devDependency, but update the version due to a security advisory. Refs: GHSA-q2c6-c6pm-g3gh * docs: ✏️ improve docs for server.rules() (#4141) * docs: ✏️ improve docs for server.rules() server.rules() was missing a good example and explanation was a bit obscure. * docs: ✏️ update requested changes for route rules Co-authored-by: Danilo Alonso <[email protected]> * Use response instead of request when marshalling (#4162) * Use response instead of request when marshalling. Closes #4161 * Cleanup * Don't change internals.fail() * Fix info.responding being set on remotely closed requests * More cleanup * 20.0.1 * upgrade to labv24 (#4167) * Update joi documentation link (#4174) Co-authored-by: cjihrig <[email protected]> Co-authored-by: Danilo Alonso <[email protected]> Co-authored-by: Danilo Alonso <[email protected]> Co-authored-by: Gil Pedersen <[email protected]> Co-authored-by: Devin Ivy <[email protected]> Co-authored-by: Lloyd Benson <[email protected]>
3 participants
server.rules() was missing a good example and explanation was a bit obscure