-
Notifications
You must be signed in to change notification settings - Fork 12
feat: add generate options #150
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
base: master
Are you sure you want to change the base?
Conversation
3728202
to
52ca111
Compare
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.
Thanks for this PR!
Overall I'm positive to this change, but left some comments for improvments and questions.
// This is optional | ||
type RequestOptions = { | ||
useCache?: boolean; | ||
} |
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.
Where does this come from?
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.
Ah I see it now, see comment on the use_body_string
option.
// Generate comments as multiline comments. | ||
// multiline comments: /** ... */ | ||
// single line comments: // ... | ||
use_multi_line_comment: 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.
Why does the comment formatting need to be configurable?
I'd rather pick one of the formats (single/multi) and always use that, not configurable.
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 set to true, body will be JSON.stringify before send | ||
// default true | ||
use_body_stringify: 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.
While I'm all for having this option, I understand it as you are also using that as a way to pass options to the request handler?
I would rather see a new argument to the request handler - something like handlerOptions: T
- than spreading it into the request body.
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.
noop, at this monent, JSON.stringify
is a mandatory behavior.
protoc-gen-typescript-http/internal/plugin/servicegen.go
Lines 167 to 170 in d11ee37
f.P(t(3), "const body = JSON.stringify(request);") | |
default: | |
nullPath := nullPropagationPath(httprule.FieldPath{rule.Body}, input) | |
f.P(t(3), "const body = JSON.stringify(request?.", nullPath, " ?? {});") |
add this option is remove it, and the default value is true
to avoid breaking change.
if someone like to use JSON.stringify
, this option can be enabled, or archive it in requestHandler
.
// If set to true, field int64 and uint64 will convert to string | ||
force_long_as_string: 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.
Should this just be called long_as_string
instead? Not sure where the force comes from ;)
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.
This name is borrowed from: https://github.com/stephenh/ts-proto
I would like to borrow from the existing naming rather than define a new one.
Of course, if you really don't like it, I can change it.
52ca111
to
0184e35
Compare
make generate configurable
Inspirit from: