Skip to content

Commit

Permalink
Rename forceOptionals=true to useOptionals=all.
Browse files Browse the repository at this point in the history
  • Loading branch information
sgielen committed Dec 10, 2021
1 parent cc8a29f commit a4f16b7
Show file tree
Hide file tree
Showing 11 changed files with 50 additions and 20 deletions.
1 change: 0 additions & 1 deletion integration/force-optionals/parameters.txt

This file was deleted.

Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { OptionalsTest, StateEnum } from './test'

describe('forceOptionals', () => {
describe('useOptionals=all', () => {
it('has all optional members', () => {
const test: OptionalsTest = {};
const data = OptionalsTest.encode(test).finish();
Expand Down
1 change: 1 addition & 0 deletions integration/use-optionals-all/parameters.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
useOptionals=all
File renamed without changes.
File renamed without changes.
File renamed without changes.
21 changes: 16 additions & 5 deletions src/main.ts
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,14 @@ import { generateGenericServiceDefinition } from './generate-generic-service-def
export function generateFile(ctx: Context, fileDesc: FileDescriptorProto): [string, Code] {
const { options, utils } = ctx;

if (options.useOptionals === false) {
console.warn("ts-proto: Passing useOptionals as a boolean option is deprecated and will be removed in a future version. Please pass the string 'none' instead of false.");
options.useOptionals = 'none';
} else if(options.useOptionals === true) {
console.warn("ts-proto: Passing useOptionals as a boolean option is deprecated and will be removed in a future version. Please pass the string 'messages' instead of true.");
options.useOptionals = 'messages';
}

// Google's protofiles are organized like Java, where package == the folder the file
// is in, and file == a specific service within the package. I.e. you can have multiple
// company/foo.proto and company/bar.proto files, where package would be 'company'.
Expand Down Expand Up @@ -547,17 +555,20 @@ function makeTimestampMethods(options: Options, longs: ReturnType<typeof makeLon
return { toTimestamp, fromTimestamp, fromJsonTimestamp };
}

// When useOptionals=true, non-scalar fields are translated into optional properties.
// When forceOptionals=true, all fields are translated into optional
// properties, with the exception of map Entry key/values, which must always be present.
// When useOptionals='messages', non-scalar fields are translated into optional
// properties. When useOptionals='all', all fields are translated into
// optional properties, with the exception of map Entry key/values, which must
// always be present.
function isOptionalProperty(
field: FieldDescriptorProto,
messageOptions: MessageOptions | undefined,
options: Options
): boolean {
const optionalMessages = options.useOptionals === true || options.useOptionals === 'messages' || options.useOptionals === 'all';
const optionalAll = options.useOptionals === 'all';
return (
(options.useOptionals && isMessage(field) && !isRepeated(field)) ||
(options.forceOptionals && !messageOptions?.mapEntry) ||
(optionalMessages && isMessage(field) && !isRepeated(field)) ||
(optionalAll && !messageOptions?.mapEntry) ||
field.proto3Optional
);
}
Expand Down
6 changes: 2 additions & 4 deletions src/options.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,7 @@ export type Options = {
context: boolean;
snakeToCamel: boolean;
forceLong: LongOption;
useOptionals: boolean;
forceOptionals: boolean;
useOptionals: boolean | 'none' | 'messages' | 'all'; // boolean is deprecated
useDate: DateOption;
oneof: OneofOption;
esModuleInterop: boolean;
Expand Down Expand Up @@ -65,8 +64,7 @@ export function defaultOptions(): Options {
context: false,
snakeToCamel: true,
forceLong: LongOption.NUMBER,
useOptionals: false,
forceOptionals: false,
useOptionals: 'none',
useDate: DateOption.DATE,
oneof: OneofOption.PROPERTIES,
esModuleInterop: false,
Expand Down
15 changes: 8 additions & 7 deletions src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -476,11 +476,11 @@ export function messageToTypeName(
// them to basic built-in types, we union the type with undefined to
// indicate the value is optional. Exceptions:
// - If the field is repeated, values cannot be undefined.
// - If useOptionals=true, all non-scalar types are already optional
// properties, so there's no need for that union.
// - If useOptionals='messages' or useOptionals='all', all non-scalar types
// are already optional properties, so there's no need for that union.
let valueType = valueTypeName(ctx, protoType);
if (!typeOptions.keepValueType && valueType) {
if (!!typeOptions.repeated || options.useOptionals) {
if (!!typeOptions.repeated || options.useOptionals === true || options.useOptionals === 'messages' || options.useOptionals === 'all') {
return valueType;
}
return code`${valueType} | undefined`;
Expand Down Expand Up @@ -527,19 +527,20 @@ export function toTypeName(ctx: Context, messageDesc: DescriptorProto, field: Fi
return type;
}

// By default (useOptionals=false, oneof=properties), non-scalar fields
// By default (useOptionals='none', oneof=properties), non-scalar fields
// outside oneofs and all fields within a oneof clause need to be unioned
// with `undefined` to indicate the value is optional.
//
// When useOptionals=true, non-scalar fields are translated to optional
// properties, so no need for the union with `undefined` here.
// When useOptionals='messages' or useOptionals='all', non-scalar fields are
// translated to optional properties, so no need for the union with
// `undefined` here.
//
// When oneof=unions, we generate a single property for the entire `oneof`
// clause, spelling each option out inside a large type union. No need for
// union with `undefined` here, either.
const { options } = ctx;
if (
(!isWithinOneOf(field) && isMessage(field) && !options.useOptionals) ||
(!isWithinOneOf(field) && isMessage(field) && (options.useOptionals === false || options.useOptionals === 'none')) ||
(isWithinOneOf(field) && options.oneof === OneofOption.PROPERTIES) ||
(isWithinOneOf(field) && field.proto3Optional)
) {
Expand Down
17 changes: 15 additions & 2 deletions tests/options-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ describe('options', () => {
"exportCommonSymbols": true,
"fileSuffix": "",
"forceLong": "number",
"forceOptionals": false,
"lowerCaseServiceMethods": true,
"nestJs": true,
"oneof": "properties",
Expand All @@ -31,7 +30,7 @@ describe('options', () => {
"stringEnums": false,
"unrecognizedEnum": true,
"useDate": "timestamp",
"useOptionals": false,
"useOptionals": "none",
}
`);
});
Expand Down Expand Up @@ -64,4 +63,18 @@ describe('options', () => {
outputServices: ServiceOption.GRPC,
});
});

it('can set useOptionals to boolean', () => {
const options = optionsFromParameter('useOptionals=true');
expect(options).toMatchObject({
useOptionals: true,
});
});

it('can set useOptionals to string', () => {
const options = optionsFromParameter('useOptionals=messages');
expect(options).toMatchObject({
useOptionals: 'messages',
});
});
});
7 changes: 7 additions & 0 deletions tests/types-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,13 @@ describe('types', () => {
options: { ...defaultOptions(), useOptionals: true },
expected: code`string`,
},
{
descr: 'value types (useOptionals="all")',
typeMap: new Map(),
protoType: '.google.protobuf.StringValue',
options: { ...defaultOptions(), useOptionals: "all" },
expected: code`string`,
},
];
testCases.forEach((t) =>
it(t.descr, async () => {
Expand Down

0 comments on commit a4f16b7

Please sign in to comment.