Add policy event

[jonathansamines]

A event based approach to solve #4104 as suggested in #4210

[Nargonath]

LGTM

[kanongil]

This is great. But there is one issue, which is that the policy event will also be emitted for server method caching.

Ie. either the docs need to be revised, or the implementation made to match the docs. I don't have an opinion on which approach will be better, so I hope you can offer which will work best for you. If you go with both, you could emit them on separate channels to differentiate it.

[kanongil]

I would also like to note that adding a built-in event is potentially a breaking change since server.events is a public interface, and it could conflict with code that adds their own policy event.

That being said, this is a rarely used feature so a conflict is unlikely and if it does occur, it would most likely result in a very obvious error during server instantiation. As such, I would probably allow it without a major revision bump.

[Nargonath]

Ie. either the docs need to be revised, or the implementation made to match the docs. I don't have an opinion on which approach will be better, so I hope you can offer which will work best for you. If you go with both, you could emit them on separate channels to differentiate it.

How would you go about differenciating them @kanongil in case you had something in mind?

[jonathansamines]

Ie. either the docs need to be revised, or the implementation made to match the docs. I don't have an opinion on which approach will be better, so I hope you can offer which will work best for you. If you go with both, you could emit them on separate channels to differentiate it.

Documentation updated. I am not sure how useful is to emit the event on separate channels though. As long as there is a policy involved, it wouldn't matter much how was it created. Thoughts?

[kanongil]

Great. If you don't need to distinguish them, then we can leave it as is. Channels / tags can always be added later when needed.

Only missing a test for triggering the policy event for a cached server method now.

[jonathansamines]

Tests for cached server methods added. CI failures seem unrelated to these changes.

[devinivy]

Looks good 👍 I re-ran the tests, noting the flaky failures for further inspection, and everything is passing. Thanks for the careful work on this, everyone. I know several folks were following along who were not actively commenting on the related issues, self included.

[devinivy]

Apologies for the late thought and potential request, but how do folks feel about changing the event name to 'cachePolicy' to avoid ambiguity? The term "policy" has meaning in catbox, but otherwise it is a generic term that might be hard to understand for users, especially because this term isn't referenced in the hapi API. @nlf brought this up originally in conversation and might have further thoughts.

[kanongil]

I agree with your points, and had a similar idea for the name. I settled on 'policy' for consistence with the existing hapi apis that seem to prefer succinct names whenever possible, and assumed that it would be challenged if anyone felt that it was an issue.

Given that this will be a rarely used event, cachePolicy should work fine, and makes it even more unlikely that it conflicts. Camel casing seems appropriate, and mirrors how node itself does it.

[Nargonath]

I agree with both of you. Renaming it is a good idea.

[jonathansamines]

Agreed. Changes applied.

[devinivy]

Beautiful. Thanks for all the careful work @jonathansamines 🙏