feat: Add js type support #1030
Conversation
dasco144
force-pushed
the
add-js_type-support
branch
4 times, most recently
from
bba5cb0
to
a53ca78
Compare
| // @ts-ignore | ||
| import Long = require("long"); |
There was a problem hiding this comment.
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.
dasco144
force-pushed
the
add-js_type-support
branch
from
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 💪 |
| 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.
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.
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.
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.
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