Skip to content
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

fix: simplify handling useJsonWireFormat=true and fix onlyTypes=true #583

Merged
merged 5 commits into from
Jun 3, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 1 addition & 7 deletions README.markdown
Original file line number Diff line number Diff line change
Expand Up @@ -396,7 +396,7 @@ Generated code will be placed in the Gradle build directory.

- With `--ts_proto_opt=onlyTypes=true`, only types will be emitted, and imports for `long` and `protobufjs/minimal` will be excluded.

Note: _This is a combination_ of `outputJsonMethods=false,outputEncodeMethods=false,outputClientImpl=false,nestJs=false`
This is the same as setting `outputJsonMethods=false,outputEncodeMethods=false,outputClientImpl=false,nestJs=false`
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if nestJs=false should be included here, as it is false by default, but nestJs=true -> onlyTypes=false.


- With `--ts_proto_opt=usePrototypeForDefaults=true`, the generated code will wrap new objects with `Object.create`.

Expand All @@ -411,12 +411,6 @@ Generated code will be placed in the Gradle build directory.
Requires `onlyTypes=true`. Implies `useDate=string` and `stringEnums=true`. This option is to generate types that can be directly used with marshalling/unmarshalling Protobuf messages serialized as JSON.
You may also want to set `useOptionals=all`, as gRPC gateways are not required to send default value for scalar values.

### Only Types

If you're looking for `ts-proto` to generate only types for your Protobuf types then passing all three of `outputEncodeMethods`, `outputJsonMethods`, and `outputClientImpl` as `false` is probably what you want, i.e.:

`--ts_proto_opt=onlyTypes=true`.

### NestJS Support

We have a great way of working together with [nestjs](https://docs.nestjs.com/microservices/grpc). `ts-proto` generates `interfaces` and `decorators` for you controller, client. For more information see the [nestjs readme](NESTJS.markdown).
Expand Down
9 changes: 4 additions & 5 deletions src/enums.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,20 +27,19 @@ export function generateEnum(
}

const delimiter = options.enumsAsLiterals ? ':' : '=';
const stringEnums = options.stringEnums || (options.onlyTypes && options.useJsonWireFormat);

enumDesc.value.forEach((valueDesc, index) => {
const info = sourceInfo.lookup(Fields.enum.value, index);
maybeAddComment(info, chunks, valueDesc.options?.deprecated, `${valueDesc.name} - `);
chunks.push(
code`${valueDesc.name} ${delimiter} ${stringEnums ? `"${valueDesc.name}"` : valueDesc.number.toString()},`
code`${valueDesc.name} ${delimiter} ${options.stringEnums ? `"${valueDesc.name}"` : valueDesc.number.toString()},`
);
});

if (options.unrecognizedEnum)
chunks.push(code`
${UNRECOGNIZED_ENUM_NAME} ${delimiter} ${
stringEnums ? `"${UNRECOGNIZED_ENUM_NAME}"` : UNRECOGNIZED_ENUM_VALUE.toString()
options.stringEnums ? `"${UNRECOGNIZED_ENUM_NAME}"` : UNRECOGNIZED_ENUM_VALUE.toString()
},`);

if (options.enumsAsLiterals) {
Expand All @@ -51,15 +50,15 @@ export function generateEnum(
chunks.push(code`}`);
}

if (options.outputJsonMethods || (stringEnums && options.outputEncodeMethods)) {
if (options.outputJsonMethods || (options.stringEnums && options.outputEncodeMethods)) {
chunks.push(code`\n`);
chunks.push(generateEnumFromJson(ctx, fullName, enumDesc));
}
if (options.outputJsonMethods) {
chunks.push(code`\n`);
chunks.push(generateEnumToJson(ctx, fullName, enumDesc));
}
if (stringEnums && options.outputEncodeMethods) {
if (options.stringEnums && options.outputEncodeMethods) {
chunks.push(code`\n`);
chunks.push(generateEnumToNumber(ctx, fullName, enumDesc));
}
Expand Down
26 changes: 23 additions & 3 deletions src/options.ts
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,6 @@ export type Options = {
unrecognizedEnum: boolean;
exportCommonSymbols: boolean;
outputSchema: boolean;
// An alias of !output
onlyTypes: boolean;
emitImportedFiles: boolean;
useExactTypes: boolean;
Expand Down Expand Up @@ -124,8 +123,18 @@ export function optionsFromParameter(parameter: string | undefined): Options {
}
Object.assign(options, parsed);
}
// We should promote onlyTypes to its own documented flag, but just an alias for now
if (!options.outputJsonMethods && !options.outputEncodeMethods && !options.outputClientImpl && !options.nestJs) {
// onlyTypes=true implies outputJsonMethods=false,outputEncodeMethods=false,outputClientImpl=false,nestJs=false
if (options.onlyTypes) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...huh, yeah, not sure why we weren't doing this before. Great fix!

If you want to add test cases to options-test.ts to make sure these don't regress, that'd be great!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably have been fixed when the option was documented. :)

I'll add test cases!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests added.

options.outputJsonMethods = false;
options.outputEncodeMethods = false;
options.outputClientImpl = false;
options.nestJs = false;
} else if (
!options.outputJsonMethods &&
!options.outputEncodeMethods &&
!options.outputClientImpl &&
!options.nestJs
) {
options.onlyTypes = true;
}

Expand Down Expand Up @@ -164,6 +173,17 @@ export function optionsFromParameter(parameter: string | undefined): Options {
options.snakeToCamel = [options.snakeToCamel];
}

if (options.useJsonWireFormat) {
if (!options.onlyTypes) {
// useJsonWireFormat requires onlyTypes=true
options.useJsonWireFormat = false;
} else {
// useJsonWireFormat implies stringEnums=true and useDate=string
options.stringEnums = true;
options.useDate = DateOption.STRING;
}
}

return options;
}

Expand Down
13 changes: 8 additions & 5 deletions src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -462,7 +462,6 @@ export function isEmptyType(typeName: string): boolean {
}

export function valueTypeName(ctx: Context, typeName: string): Code | undefined {
const useJsonWireFormat = ctx.options.onlyTypes && ctx.options.useJsonWireFormat;
switch (typeName) {
case '.google.protobuf.StringValue':
return code`string`;
Expand All @@ -478,19 +477,23 @@ export function valueTypeName(ctx: Context, typeName: string): Code | undefined
case '.google.protobuf.BoolValue':
return code`boolean`;
case '.google.protobuf.BytesValue':
return ctx.options.env === EnvOption.NODE ? code`Buffer` : useJsonWireFormat ? code`string` : code`Uint8Array`;
return ctx.options.env === EnvOption.NODE
? code`Buffer`
: ctx.options.useJsonWireFormat
? code`string`
: code`Uint8Array`;
case '.google.protobuf.ListValue':
return code`Array<any>`;
case '.google.protobuf.Value':
return code`any`;
case '.google.protobuf.Struct':
return code`{[key: string]: any}`;
case '.google.protobuf.FieldMask':
return useJsonWireFormat ? code`string` : code`string[]`;
return ctx.options.useJsonWireFormat ? code`string` : code`string[]`;
case '.google.protobuf.Duration':
return useJsonWireFormat ? code`string` : undefined;
return ctx.options.useJsonWireFormat ? code`string` : undefined;
case '.google.protobuf.Timestamp':
return useJsonWireFormat ? code`string` : undefined;
return ctx.options.useJsonWireFormat ? code`string` : undefined;
default:
return undefined;
}
Expand Down
36 changes: 35 additions & 1 deletion tests/options-test.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { optionsFromParameter, ServiceOption } from '../src/options';
import { DateOption, optionsFromParameter, ServiceOption } from '../src/options';

describe('options', () => {
it('can set outputJsonMethods with nestJs=true', () => {
Expand Down Expand Up @@ -99,4 +99,38 @@ describe('options', () => {
const options = optionsFromParameter('foo=one,foo=two');
expect(options).toMatchObject({ foo: ['one', 'two'] });
});

it('´output*=false implies onlyTypes', () => {
const options = optionsFromParameter('outputJsonMethods=false,outputEncodeMethods=false,outputClientImpl=false');
expect(options).toMatchObject({
onlyTypes: true,
});
});

it('onlyTypes implies output*=false', () => {
const options = optionsFromParameter('onlyTypes=true');
expect(options).toMatchObject({
outputJsonMethods: false,
outputEncodeMethods: false,
outputClientImpl: false,
nestJs: false,
});
});

it('useJsonWireFormat requires onlyTypes', () => {
const options = optionsFromParameter('useJsonWireFormat=true');
expect(options).toMatchObject({
useJsonWireFormat: false,
});
});

it('useJsonWireFormat implies useDate=string and stringEnums=true', () => {
const options = optionsFromParameter('useJsonWireFormat=true,onlyTypes=true');
expect(options).toMatchObject({
useJsonWireFormat: true,
onlyTypes: true,
stringEnums: true,
useDate: DateOption.STRING,
});
});
});