Fix get/set conflicting method names. #272
Conversation
improbable-prow-robot
added
do-not-merge/hold
| * optional string extension = 1; | ||
| * @return {string} | ||
| */ | ||
| proto.examplecom.GetterNameConflictMessage.prototype.getExtension$ = function() { |
There was a problem hiding this comment.
Here you can see the protoc compiler adds $ to get and set functions.
| * optional bytes js_pb_message_id = 2; | ||
| * @return {!(string|Uint8Array)} | ||
| */ | ||
| proto.examplecom.GetterNameConflictMessage.prototype.getJsPbMessageId$ = function() { |
There was a problem hiding this comment.
It does the same for bytes get/set methods.
| printer.printIndentedLn(`get${withUppercase}(): ${exportType}${canBeUndefined ? " | undefined" : ""};`); | ||
| printer.printIndentedLn(`set${withUppercase}(value${type === MESSAGE_TYPE ? "?" : ""}: ${exportType}): void;`); | ||
| printer.printIndentedLn(`get${jsGetterName(withUppercase)}(): ${exportType}${canBeUndefined ? " | undefined" : ""};`); | ||
| printer.printIndentedLn(`set${jsGetterName(withUppercase)}(value${type === MESSAGE_TYPE ? "?" : ""}: ${exportType}): void;`); |
There was a problem hiding this comment.
I tried to also verify if field presence is enabled, but the TS generation fails. It is possible there might be conflicts with additional methods (hasExtension, etc.) when it is supported.
There was a problem hiding this comment.
I think you were hitting this issue which is now fixed on master.
I've added the presence checks in this commit. Can you add them to this PR please?
There was a problem hiding this comment.
I've rebased to master and pulled in that change - thanks!
|
/assign @easyCZ |
|
@MarcusLongmuir - Is there any additional steps I should follow for PR reviews or is assigning @easyCZ still the correct process? |
improbable-prow-robot
added
lgtm
Update the typescript generator to add a suffix '$' to methods which
conflict on the Message base class
('getExtension'/'setExtension'/'getJsPbMessageId'/'setJsPbMessageId').
This will keep the typescript definitions in sync with the generated JS
code from the protoc compiler.
Fixes #271.
| @@ -157,12 +163,12 @@ export function printMessage(fileName: string, exportMap: ExportMap, messageDesc | |||
| function printClearIfNotPresent() { | |||
| if (!hasClearMethod) { | |||
| hasClearMethod = true; | |||
| printer.printIndentedLn(`clear${withUppercase}${field.getLabel() === FieldDescriptorProto.Label.LABEL_REPEATED ? "List" : ""}(): void;`); | |||
| printer.printIndentedLn(`clear${jsGetterName(withUppercase)}${field.getLabel() === FieldDescriptorProto.Label.LABEL_REPEATED ? "List" : ""}(): void;`); | |||
There was a problem hiding this comment.
Noticed that the clear method name didn't match the generated code. This is now fixed as well (and tests updated).
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: MarcusLongmuir, moadz The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Update the typescript generator to add a suffix '$' to methods which
conflict on the Message base class
('getExtension'/'setExtension'/'getJsPbMessageId'/'setJsPbMessageId').
This will keep the typescript definitions in sync with the generated JS
code from the protoc compiler.
Fixes #271.
Changes
Verification