-
Notifications
You must be signed in to change notification settings - Fork 350
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
feat: Add js type support #1030
Conversation
bba5cb0
to
a53ca78
Compare
// @ts-ignore | ||
import Long = require("long"); |
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'm not sure how to resolve this at the moment. In my editor it errors, stating that the tsconfig esModuleInterop
is enabled for this file and I can't use an Import assignment, but from what I can see at runtime it does not have esModuleInterop
enabled.
If we import using a default import
import Long from "long";
then the editor no longer reports an error, but tests fail due to it not being able to resolve the import.
be024fb
to
d1ed693
Compare
Hi @dasco144 , thanks for the PR! Just wanted to say it'll probably be ~few days/this weekend before I get a chance to look at it. |
Thanks! Let me know if I need to change anything 💪 |
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.
Hi @dasco144 , this is great! The test coverage is especially amazing, and appreciated! Just a few questions, mostly for my own understanding, but otherwise lgtm!
case FieldOptions_JSType.JS_STRING: | ||
return FieldDescriptorProto_Type.TYPE_STRING; | ||
case FieldOptions_JSType.JS_NUMBER: | ||
return FieldDescriptorProto_Type.TYPE_INT64; |
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.
Do we need to handle the JS_NORMAL value here?
https://googleapis.dev/nodejs/nodejs-functions/latest/google.protobuf.FieldOptions.html
If not, maybe put an in-source comment about why not? Like maybe if JS_NORMAL
is used, the jstype
ends up being undefined
because JS_NORMAL
is an 0th value enum (just guessing!)...
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.
From my understanding, the JS_NORMAL
case would be equivalent to not having the JSType
set on the field at all, so we want the behaviour to match the same case as if it were undefined. That's why I made it fallthrough and default to not returning an explicit value.
I'll add a quick comment explaining this 👍
I could also maybe move it into the guard clause above
if ( | ||
(isLong(field) || isLongValueType(field)) && | ||
options.forceLong === LongOption.LONG && | ||
!isJsTypeFieldOption(options, field) |
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.
Just curious, but we don't have any other isJsTypeFieldOption
checks in here, but I imagine maybe that's because the types will line-up (for anything not this forceLong case?) and so we don't need any special handling?
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.
Yeah, exactly that. These cases will fall through to the isPrimitive
check below, and just output reading from the variable as is (without any special method calls or handling)
Sweet, this is amazing @dasco144 , thank you! |
🎉 This PR is included in version 1.173.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
I realised I did not update the |
Closes #958