From d939f1e68126502de88bdfc49cac463b2b8014a3 Mon Sep 17 00:00:00 2001 From: Andres Correa Casablanca Date: Fri, 8 Oct 2021 10:37:08 +0200 Subject: [PATCH] fix: extend previous fix for deprecated fields --- internal/generator/runner_generate_class.go | 5 +- tests/__tests__/generated/Regressions.ts | 136 +++++++++++--------- tests/__tests__/generated/Test.ts | 3 + tests/__tests__/generated/common/Common.ts | 3 + tests/__tests__/generated/common/Extra.ts | 3 + tests/__tests__/proto/regressions.proto | 4 +- tests/__tests__/regressions.test.ts | 53 ++++++-- 7 files changed, 125 insertions(+), 82 deletions(-) diff --git a/internal/generator/runner_generate_class.go b/internal/generator/runner_generate_class.go index 51fa027..c48dc32 100644 --- a/internal/generator/runner_generate_class.go +++ b/internal/generator/runner_generate_class.go @@ -138,7 +138,7 @@ func (r *Runner) generateTypescriptMessageClass(generatedFileStream *protogen.Ge r.indentLevel += 2 for _, fieldSpec := range messageSpec.GetField() { - r.generateTypescriptClassField(generatedFileStream, fieldSpec, messageSpec, messageOptions, requiredFields) + r.generateTypescriptClassField(generatedFileStream, fieldSpec, messageSpec, requiredFields) } r.generateTypescriptClassPatchedMethods(generatedFileStream, messageSpec, requiredFields, hasEnums) @@ -151,12 +151,11 @@ func (r *Runner) generateTypescriptClassField( generatedFileStream *protogen.GeneratedFile, fieldSpec *descriptorpb.FieldDescriptorProto, messageSpec *descriptorpb.DescriptorProto, - messageOptions *descriptorpb.MessageOptions, requiredFields bool, ) { fieldOptions := fieldSpec.GetOptions() if fieldOptions != nil { - if messageOptions.GetDeprecated() { + if fieldOptions.GetDeprecated() { r.P(generatedFileStream, "/**\n * @deprecated\n */") } } diff --git a/tests/__tests__/generated/Regressions.ts b/tests/__tests__/generated/Regressions.ts index bbd24ec..679bf57 100644 --- a/tests/__tests__/generated/Regressions.ts +++ b/tests/__tests__/generated/Regressions.ts @@ -17,7 +17,7 @@ export namespace Regressions { inner?: IReg01Inner } - export interface IWithDeprecatedField { + export interface IMessageWithDeprecatedField { notDeprecated?: string /** * @deprecated @@ -28,7 +28,7 @@ export namespace Regressions { /** * @deprecated */ - export interface IDeprecatedWithDeprecatedField { + export interface IDeprecatedMessageWithDeprecatedField { notDeprecated?: string /** * @deprecated @@ -39,12 +39,12 @@ export namespace Regressions { /** * @deprecated */ - @protobufjs.Type.d('regressions_DeprecatedWithDeprecatedField') - export class DeprecatedWithDeprecatedField - extends protobufjs.Message + @protobufjs.Type.d('regressions_DeprecatedMessageWithDeprecatedField') + export class DeprecatedMessageWithDeprecatedField + extends protobufjs.Message implements - ConvertibleTo, - IDeprecatedWithDeprecatedField + ConvertibleTo, + IDeprecatedMessageWithDeprecatedField { @protobufjs.Field.d(1, 'string', 'optional') public notDeprecated?: string @@ -55,16 +55,19 @@ export namespace Regressions { @protobufjs.Field.d(2, 'string', 'optional') public deprecated?: string - public asInterface(): IDeprecatedWithDeprecatedField { + public asInterface(): IDeprecatedMessageWithDeprecatedField { const message = { ...this, } for (const fieldName of Object.keys(message)) { if ( - message[fieldName as keyof IDeprecatedWithDeprecatedField] == null + message[fieldName as keyof IDeprecatedMessageWithDeprecatedField] == + null ) { // We remove the key to avoid problems with code making too many assumptions - delete message[fieldName as keyof IDeprecatedWithDeprecatedField] + delete message[ + fieldName as keyof IDeprecatedMessageWithDeprecatedField + ] } } return message @@ -72,24 +75,76 @@ export namespace Regressions { public static fromInterface( this: void, - value: IDeprecatedWithDeprecatedField - ): DeprecatedWithDeprecatedField { - return DeprecatedWithDeprecatedField.fromObject(value) + value: IDeprecatedMessageWithDeprecatedField + ): DeprecatedMessageWithDeprecatedField { + return DeprecatedMessageWithDeprecatedField.fromObject(value) } public static decodePatched( this: void, reader: protobufjs.Reader | Uint8Array - ): IDeprecatedWithDeprecatedField { - return DeprecatedWithDeprecatedField.decode(reader).asInterface() + ): IDeprecatedMessageWithDeprecatedField { + return DeprecatedMessageWithDeprecatedField.decode(reader).asInterface() } public static encodePatched( this: void, - message: IDeprecatedWithDeprecatedField, + message: IDeprecatedMessageWithDeprecatedField, writer?: protobufjs.Writer ): protobufjs.Writer { - return DeprecatedWithDeprecatedField.encode(message, writer) + return DeprecatedMessageWithDeprecatedField.encode(message, writer) + } + } + + @protobufjs.Type.d('regressions_MessageWithDeprecatedField') + export class MessageWithDeprecatedField + extends protobufjs.Message + implements + ConvertibleTo, + IMessageWithDeprecatedField + { + @protobufjs.Field.d(1, 'string', 'optional') + public notDeprecated?: string + + /** + * @deprecated + */ + @protobufjs.Field.d(2, 'string', 'optional') + public deprecated?: string + + public asInterface(): IMessageWithDeprecatedField { + const message = { + ...this, + } + for (const fieldName of Object.keys(message)) { + if (message[fieldName as keyof IMessageWithDeprecatedField] == null) { + // We remove the key to avoid problems with code making too many assumptions + delete message[fieldName as keyof IMessageWithDeprecatedField] + } + } + return message + } + + public static fromInterface( + this: void, + value: IMessageWithDeprecatedField + ): MessageWithDeprecatedField { + return MessageWithDeprecatedField.fromObject(value) + } + + public static decodePatched( + this: void, + reader: protobufjs.Reader | Uint8Array + ): IMessageWithDeprecatedField { + return MessageWithDeprecatedField.decode(reader).asInterface() + } + + public static encodePatched( + this: void, + message: IMessageWithDeprecatedField, + writer?: protobufjs.Writer + ): protobufjs.Writer { + return MessageWithDeprecatedField.encode(message, writer) } } @@ -175,51 +230,4 @@ export namespace Regressions { return Reg01Outer.encode(message, writer) } } - - @protobufjs.Type.d('regressions_WithDeprecatedField') - export class WithDeprecatedField - extends protobufjs.Message - implements ConvertibleTo, IWithDeprecatedField - { - @protobufjs.Field.d(1, 'string', 'optional') - public notDeprecated?: string - - @protobufjs.Field.d(2, 'string', 'optional') - public deprecated?: string - - public asInterface(): IWithDeprecatedField { - const message = { - ...this, - } - for (const fieldName of Object.keys(message)) { - if (message[fieldName as keyof IWithDeprecatedField] == null) { - // We remove the key to avoid problems with code making too many assumptions - delete message[fieldName as keyof IWithDeprecatedField] - } - } - return message - } - - public static fromInterface( - this: void, - value: IWithDeprecatedField - ): WithDeprecatedField { - return WithDeprecatedField.fromObject(value) - } - - public static decodePatched( - this: void, - reader: protobufjs.Reader | Uint8Array - ): IWithDeprecatedField { - return WithDeprecatedField.decode(reader).asInterface() - } - - public static encodePatched( - this: void, - message: IWithDeprecatedField, - writer?: protobufjs.Writer - ): protobufjs.Writer { - return WithDeprecatedField.encode(message, writer) - } - } } diff --git a/tests/__tests__/generated/Test.ts b/tests/__tests__/generated/Test.ts index 9876314..ab850dc 100644 --- a/tests/__tests__/generated/Test.ts +++ b/tests/__tests__/generated/Test.ts @@ -376,6 +376,9 @@ export namespace Foo { @protobufjs.Field.d(12, 'sint32', 'repeated') public fieldSint32Repeated?: number[] + /** + * @deprecated + */ @protobufjs.Field.d(13, 'sint64', 'optional') public fieldSint64?: number diff --git a/tests/__tests__/generated/common/Common.ts b/tests/__tests__/generated/common/Common.ts index df7fd58..edf5aa0 100644 --- a/tests/__tests__/generated/common/Common.ts +++ b/tests/__tests__/generated/common/Common.ts @@ -22,6 +22,9 @@ export namespace Common { extends protobufjs.Message implements ConvertibleTo, IOtherPkgMessage { + /** + * @deprecated + */ @protobufjs.Field.d(1, 'string', 'optional') public firstName?: string diff --git a/tests/__tests__/generated/common/Extra.ts b/tests/__tests__/generated/common/Extra.ts index ef0af81..7b9155f 100644 --- a/tests/__tests__/generated/common/Extra.ts +++ b/tests/__tests__/generated/common/Extra.ts @@ -25,6 +25,9 @@ export namespace Common { extends protobufjs.Message implements ConvertibleTo { + /** + * @deprecated + */ @protobufjs.Field.d(1, 'string', 'optional') public firstName?: string diff --git a/tests/__tests__/proto/regressions.proto b/tests/__tests__/proto/regressions.proto index 0b3f481..9354fe3 100644 --- a/tests/__tests__/proto/regressions.proto +++ b/tests/__tests__/proto/regressions.proto @@ -20,12 +20,12 @@ message Reg01Outer { // Regression 02 // ---------------------------------------------------------------------------- -message WithDeprecatedField { +message MessageWithDeprecatedField { string not_deprecated = 1; string deprecated = 2 [deprecated=true]; } -message DeprecatedWithDeprecatedField { +message DeprecatedMessageWithDeprecatedField { option deprecated = true; string not_deprecated = 1; string deprecated = 2 [deprecated=true]; diff --git a/tests/__tests__/regressions.test.ts b/tests/__tests__/regressions.test.ts index ffcc9d9..9f45554 100644 --- a/tests/__tests__/regressions.test.ts +++ b/tests/__tests__/regressions.test.ts @@ -1,7 +1,7 @@ // eslint-disable-next-line node/no-unpublished-import import * as ts from 'typescript' // eslint-disable-next-line node/no-unpublished-import -import { InterfaceDeclaration, Project } from 'ts-morph' +import { ClassDeclaration, InterfaceDeclaration, Project } from 'ts-morph' import { Regressions } from './generated/Regressions' // eslint-disable-next-line node/no-unpublished-import import { parse as parseComment } from 'comment-parser' @@ -44,33 +44,60 @@ describe('regressions', () => { expect(moduleDeclaration).toBeDefined() expect(moduleDeclaration.getName()).toBe('Regressions') + // Here is where we verify the fix works as intended. + // ------------------------------------------------------------------------- // eslint-disable-next-line @typescript-eslint/no-non-null-assertion - const withDeprecatedFieldInterface = moduleDeclaration.getInterface( - 'IWithDeprecatedField' + const messageInterfaceWithDeprecatedField = moduleDeclaration.getInterface( + 'IMessageWithDeprecatedField' )! - expect(withDeprecatedFieldInterface).toBeDefined() + expect(messageInterfaceWithDeprecatedField).toBeDefined() verifyThatDeprecatedFieldHasJSDocDeprecationAnnotation( - withDeprecatedFieldInterface + messageInterfaceWithDeprecatedField ) verifyThatNotDeprecatedFieldDoesNotHaveJSDocAnnotation( - withDeprecatedFieldInterface + messageInterfaceWithDeprecatedField ) - const deprecatedWithDeprecatedFieldInterface = + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion + const messageClassWithDeprecatedField = moduleDeclaration.getClass( + 'MessageWithDeprecatedField' + )! + expect(messageClassWithDeprecatedField).toBeDefined() + verifyThatDeprecatedFieldHasJSDocDeprecationAnnotation( + messageClassWithDeprecatedField + ) + verifyThatNotDeprecatedFieldDoesNotHaveJSDocAnnotation( + messageClassWithDeprecatedField + ) + + // Here we verify that the fix did not introduce any unintended regression. + // ------------------------------------------------------------------------- + const deprecatedMessageInterfaceWithDeprecatedField = + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion + moduleDeclaration.getInterface('IDeprecatedMessageWithDeprecatedField')! + expect(deprecatedMessageInterfaceWithDeprecatedField).toBeDefined() + verifyThatDeprecatedFieldHasJSDocDeprecationAnnotation( + deprecatedMessageInterfaceWithDeprecatedField + ) + verifyThatNotDeprecatedFieldDoesNotHaveJSDocAnnotation( + deprecatedMessageInterfaceWithDeprecatedField + ) + + const deprecatedMessageClassWithDeprecatedField = // eslint-disable-next-line @typescript-eslint/no-non-null-assertion - moduleDeclaration.getInterface('IDeprecatedWithDeprecatedField')! - expect(deprecatedWithDeprecatedFieldInterface).toBeDefined() + moduleDeclaration.getClass('DeprecatedMessageWithDeprecatedField')! + expect(deprecatedMessageClassWithDeprecatedField).toBeDefined() verifyThatDeprecatedFieldHasJSDocDeprecationAnnotation( - deprecatedWithDeprecatedFieldInterface + deprecatedMessageClassWithDeprecatedField ) verifyThatNotDeprecatedFieldDoesNotHaveJSDocAnnotation( - deprecatedWithDeprecatedFieldInterface + deprecatedMessageClassWithDeprecatedField ) }) }) function verifyThatDeprecatedFieldHasJSDocDeprecationAnnotation( - generatedInterface: InterfaceDeclaration + generatedInterface: InterfaceDeclaration | ClassDeclaration ): void { const deprecatedField = // eslint-disable-next-line @typescript-eslint/no-non-null-assertion @@ -94,7 +121,7 @@ function verifyThatDeprecatedFieldHasJSDocDeprecationAnnotation( } function verifyThatNotDeprecatedFieldDoesNotHaveJSDocAnnotation( - generatedInterface: InterfaceDeclaration + generatedInterface: InterfaceDeclaration | ClassDeclaration ): void { const notDeprecatedField = // eslint-disable-next-line @typescript-eslint/no-non-null-assertion