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
Add capability to define optional handler behavior #444
Conversation
* add Option WithSetEscapeHTML to set this same named option on the underlying json encoder * refactor implementations of ***WithContext functions to use the Option WithContext * mark a bunch of not useful related stuff as deprecated so that they are hidden in the godocs
Codecov Report
@@ Coverage Diff @@
## main #444 +/- ##
==========================================
+ Coverage 72.01% 72.83% +0.82%
==========================================
Files 19 19
Lines 1054 1086 +32
==========================================
+ Hits 759 791 +32
- Misses 225 226 +1
+ Partials 70 69 -1
Continue to review full report at Codecov.
|
Thanks for this Bryan! It may be worth adding a little Usage section to describe how a user of the library would set some of these options |
lambda/handler.go
Outdated
type handlerOptions struct { | ||
Handler | ||
baseContext context.Context | ||
escapeHTML bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is only relevant to response serialization, maybe we can make it explicit in the property name (eg. escapeResponseHTML
or something along those lines)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. I would even go a bit further and prefix/suffix these with something like ResponseJSON
so it would be something like:
responseJSONEscapeHTML
responseJSONPrefix
responseJSONIndex
Alternatively you could have customers pass in a newJSONEncoder
function which they could configure however they like (or use a different JSON library). It would have the following signature:
newJSONEncoder func(out io.Writer) error
That might be the most flexible. Could even throw in a newJSONDecoder
so users could have complete control over the JSON part of the process.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did play around with the custom encoder idea before pushing this PR! However didn't come up with something that still elegantly worked with the stdlib json NewEncoder. My failed ideas looked something like this: https://play.golang.com/p/To4q1sKBbjm
Something like this would work:
func WithEncoder(encoderFactory func(io.Writer) func(any) error) Option {
return Option(func(h *handlerOption) {
h.encoderFactory = encoderFactory
})
}
// use example with stdlib json
encoderOption := WithEncoder(func(w io.Writer) func(any) error {
return json.NewEncoder(w).Encode
})
But i'd want to mull on it a little more, and look at other popular encoders, to see if there's not something more ergonomic that can be figured out
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea, I personally like the WithEncoder
pattern over having three new json-specific variables but either way ultimately works.
Issue #, if available:
#177
#178 (comment)
Description of changes:
This adds an options pattern for allowing a function author to pick from one or more customizations to the default runtime behavior. The pattern can later be followed to implement other requests for changes to the default runtime behavior. Example feature request: #433
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.