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.

Sign up for GitHub

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: oneof=union breaks wrapper types #458 #462

Merged
merged 11 commits into from
Jan 5, 2022
Merged

fix: oneof=union breaks wrapper types #458 #462

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
607d5cd
fix: oneof=union breaks wrapper types #458
boukeversteegh Jan 4, 2022
b95e27d
undo local changes
boukeversteegh Jan 4, 2022
0e17a41
Update struct.ts
boukeversteegh Jan 4, 2022
6f52e2c
prettier
boukeversteegh Jan 4, 2022
8fc7544
unwrap json values when json decoding in values integration test
boukeversteegh Jan 4, 2022
3d231f8
Remove unnecessary unwrapFromJSON
boukeversteegh Jan 5, 2022
a518a05
Remove unnecessary unwrapFromJSON
boukeversteegh Jan 5, 2022
f952b0f
Remove unnecessary unwrapFromJSON
boukeversteegh Jan 5, 2022
2649b9a
Fix test
boukeversteegh Jan 5, 2022
e50fbd8
Add missing struct file
boukeversteegh Jan 5, 2022
92f4a3f
Keep one line conditional assignment for array fields
boukeversteegh Jan 5, 2022
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
7 changes: 5 additions & 2 deletions integration/angular/simple-message.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,7 @@ export const SimpleMessage = {

fromJSON(object: any): SimpleMessage {
const message = createBaseSimpleMessage();
message.numberField =
object.numberField !== undefined && object.numberField !== null ? Number(object.numberField) : 0;
message.numberField = isSet(object.numberField) ? Number(object.numberField) : 0;
return message;
},

Expand Down Expand Up @@ -81,3 +80,7 @@ if (util.Long !== Long) {
util.Long = Long as any;
configure();
}

function isSet(value: any): boolean {
return value !== null && value !== undefined;
Copy link
Collaborator

@webmaster128 webmaster128 Jan 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Now that this is pulled out into a nicely named helper, you can even use value != null.

Do we need type narrowing? You can do something like

function isSet<X>(value: X  | undefined | null): value is X {
  return value != null;
}

}
17 changes: 8 additions & 9 deletions integration/avoid-import-conflicts/simple.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,11 +96,8 @@ export const Simple = {

fromJSON(object: any): Simple {
const message = createBaseSimple();
message.name = object.name !== undefined && object.name !== null ? String(object.name) : '';
message.otherSimple =
object.otherSimple !== undefined && object.otherSimple !== null
? Simple2.fromJSON(object.otherSimple)
: undefined;
message.name = isSet(object.name) ? String(object.name) : '';
message.otherSimple = isSet(object.otherSimple) ? Simple2.fromJSON(object.otherSimple) : undefined;
return message;
},

Expand Down Expand Up @@ -161,10 +158,8 @@ export const SimpleEnums = {

fromJSON(object: any): SimpleEnums {
const message = createBaseSimpleEnums();
message.localEnum =
object.localEnum !== undefined && object.localEnum !== null ? simpleEnumFromJSON(object.localEnum) : 0;
message.importEnum =
object.importEnum !== undefined && object.importEnum !== null ? simpleEnumFromJSON3(object.importEnum) : 0;
message.localEnum = isSet(object.localEnum) ? simpleEnumFromJSON(object.localEnum) : 0;
message.importEnum = isSet(object.importEnum) ? simpleEnumFromJSON3(object.importEnum) : 0;
return message;
},

Expand Down Expand Up @@ -206,3 +201,7 @@ if (util.Long !== Long) {
util.Long = Long as any;
configure();
}

function isSet(value: any): boolean {
return value !== null && value !== undefined;
}
8 changes: 6 additions & 2 deletions integration/avoid-import-conflicts/simple2.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,8 +85,8 @@ export const Simple = {

fromJSON(object: any): Simple {
const message = createBaseSimple();
message.name = object.name !== undefined && object.name !== null ? String(object.name) : '';
message.age = object.age !== undefined && object.age !== null ? Number(object.age) : 0;
message.name = isSet(object.name) ? String(object.name) : '';
message.age = isSet(object.age) ? Number(object.age) : 0;
return message;
},

Expand Down Expand Up @@ -128,3 +128,7 @@ if (util.Long !== Long) {
util.Long = Long as any;
configure();
}

function isSet(value: any): boolean {
return value !== null && value !== undefined;
}
8 changes: 6 additions & 2 deletions integration/barrel-imports/bar.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,8 @@ export const Bar = {

fromJSON(object: any): Bar {
const message = createBaseBar();
message.name = object.name !== undefined && object.name !== null ? String(object.name) : '';
message.age = object.age !== undefined && object.age !== null ? Number(object.age) : 0;
message.name = isSet(object.name) ? String(object.name) : '';
message.age = isSet(object.age) ? Number(object.age) : 0;
return message;
},

Expand Down Expand Up @@ -88,3 +88,7 @@ if (util.Long !== Long) {
util.Long = Long as any;
configure();
}

function isSet(value: any): boolean {
return value !== null && value !== undefined;
}
8 changes: 6 additions & 2 deletions integration/barrel-imports/foo.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,8 @@ export const Foo = {

fromJSON(object: any): Foo {
const message = createBaseFoo();
message.name = object.name !== undefined && object.name !== null ? String(object.name) : '';
message.bar = object.bar !== undefined && object.bar !== null ? Bar.fromJSON(object.bar) : undefined;
message.name = isSet(object.name) ? String(object.name) : '';
message.bar = isSet(object.bar) ? Bar.fromJSON(object.bar) : undefined;
return message;
},

Expand Down Expand Up @@ -89,3 +89,7 @@ if (util.Long !== Long) {
util.Long = Long as any;
configure();
}

function isSet(value: any): boolean {
return value !== null && value !== undefined;
}
30 changes: 20 additions & 10 deletions integration/batching-with-context/batching.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,9 @@ export const BatchQueryRequest = {

fromJSON(object: any): BatchQueryRequest {
const message = createBaseBatchQueryRequest();
message.ids = (object.ids ?? []).map((e: any) => String(e));
if (Array.isArray(object?.ids)) {
Copy link
Owner

Choose a reason for hiding this comment

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

I like the isSet helper method, but for the arrays, @webmaster128 had just done quite a bit of work to rework all of our assignments to be one-liners, with the idea of a) minimizing code size, and b) eventually turning these into object literals, like:

return {
  a: ...,
  b: ....,
  c: ...,
}

Which in theory should (with some handwaving) help v8 optimiziations.

Can we keep the previous ?? [] approach?

Copy link
Collaborator Author

@boukeversteegh boukeversteegh Jan 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I see, so the idea is to get rid of the createBase...() call?

It made me wonder already why the default values for each field were overwritten each time. Thanks for clarifying, now it makes more sense.

I'll change back to the ternary style.

Copy link
Owner

Choose a reason for hiding this comment

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

I see, so the idea is to get rid of the createBase...() call

Yep! @aikoven has an in-flight PR that is either doing this soon, or at least moving us closer to that I think.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@stephenh Ah I didn't know @aikoven was already working on this. So I actually just made a PR #463 to do what you described. It was kind of simple as I was already deep into this part of the code, and had a solution in mind for the trickier 'oneof' case.

So, @aikoven, if you also had something in the works and want to finish that instead, please close my PR. Sorry if I got in the way.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for adapting. Yeah the overall direction is to move away from mutability where you never know for sure what is set when. So a lot of recent work sent into exactly one assignment per field.

This PR nicely adds to that journey

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see, so the idea is to get rid of the createBase...() call?

We can easily get rid of it in fromJSON and fromPartial. For decode it will probably still be required as we are reading the serialized data into the object in unknown order and not all fields are serialized.

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah, for decode I was thinking that calling createBase() to get an object literal with every field assigned (right now I don't think it does literally every field, just some primitives), and then have the decode for loop just assign over existing keys, would be most likely to trigger the supposedly / I'm-admittedly-cargo-culting-a-little-bit optimization of "create objects with all keys set and then don't add/remove keys". :-)

Copy link
Collaborator

Choose a reason for hiding this comment

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

right now I don't think it does literally every field, just some primitives

That changed in #457, see e.g. #457 (comment). Now all fields are set to default and decode can override them.

Copy link
Owner

Choose a reason for hiding this comment

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

Ah great!

message.ids = object.ids.map((e: any) => String(e));
}
return message;
},

Expand Down Expand Up @@ -131,7 +133,9 @@ export const BatchQueryResponse = {

fromJSON(object: any): BatchQueryResponse {
const message = createBaseBatchQueryResponse();
message.entities = (object.entities ?? []).map((e: any) => Entity.fromJSON(e));
if (Array.isArray(object?.entities)) {
message.entities = object.entities.map((e: any) => Entity.fromJSON(e));
}
return message;
},

Expand Down Expand Up @@ -184,7 +188,9 @@ export const BatchMapQueryRequest = {

fromJSON(object: any): BatchMapQueryRequest {
const message = createBaseBatchMapQueryRequest();
message.ids = (object.ids ?? []).map((e: any) => String(e));
if (Array.isArray(object?.ids)) {
message.ids = object.ids.map((e: any) => String(e));
}
return message;
},

Expand Down Expand Up @@ -308,8 +314,8 @@ export const BatchMapQueryResponse_EntitiesEntry = {

fromJSON(object: any): BatchMapQueryResponse_EntitiesEntry {
const message = createBaseBatchMapQueryResponse_EntitiesEntry();
message.key = object.key !== undefined && object.key !== null ? String(object.key) : '';
message.value = object.value !== undefined && object.value !== null ? Entity.fromJSON(object.value) : undefined;
message.key = isSet(object.key) ? String(object.key) : '';
message.value = isSet(object.value) ? Entity.fromJSON(object.value) : undefined;
return message;
},

Expand Down Expand Up @@ -362,7 +368,7 @@ export const GetOnlyMethodRequest = {

fromJSON(object: any): GetOnlyMethodRequest {
const message = createBaseGetOnlyMethodRequest();
message.id = object.id !== undefined && object.id !== null ? String(object.id) : '';
message.id = isSet(object.id) ? String(object.id) : '';
return message;
},

Expand Down Expand Up @@ -411,7 +417,7 @@ export const GetOnlyMethodResponse = {

fromJSON(object: any): GetOnlyMethodResponse {
const message = createBaseGetOnlyMethodResponse();
message.entity = object.entity !== undefined && object.entity !== null ? Entity.fromJSON(object.entity) : undefined;
message.entity = isSet(object.entity) ? Entity.fromJSON(object.entity) : undefined;
return message;
},

Expand Down Expand Up @@ -461,7 +467,7 @@ export const WriteMethodRequest = {

fromJSON(object: any): WriteMethodRequest {
const message = createBaseWriteMethodRequest();
message.id = object.id !== undefined && object.id !== null ? String(object.id) : '';
message.id = isSet(object.id) ? String(object.id) : '';
return message;
},

Expand Down Expand Up @@ -556,8 +562,8 @@ export const Entity = {

fromJSON(object: any): Entity {
const message = createBaseEntity();
message.id = object.id !== undefined && object.id !== null ? String(object.id) : '';
message.name = object.name !== undefined && object.name !== null ? String(object.name) : '';
message.id = isSet(object.id) ? String(object.id) : '';
message.name = isSet(object.name) ? String(object.name) : '';
return message;
},

Expand Down Expand Up @@ -696,3 +702,7 @@ if (util.Long !== Long) {
util.Long = Long as any;
configure();
}

function isSet(value: any): boolean {
return value !== null && value !== undefined;
}
30 changes: 20 additions & 10 deletions integration/batching/batching.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,9 @@ export const BatchQueryRequest = {

fromJSON(object: any): BatchQueryRequest {
const message = createBaseBatchQueryRequest();
message.ids = (object.ids ?? []).map((e: any) => String(e));
if (Array.isArray(object?.ids)) {
message.ids = object.ids.map((e: any) => String(e));
}
return message;
},

Expand Down Expand Up @@ -129,7 +131,9 @@ export const BatchQueryResponse = {

fromJSON(object: any): BatchQueryResponse {
const message = createBaseBatchQueryResponse();
message.entities = (object.entities ?? []).map((e: any) => Entity.fromJSON(e));
if (Array.isArray(object?.entities)) {
message.entities = object.entities.map((e: any) => Entity.fromJSON(e));
}
return message;
},

Expand Down Expand Up @@ -182,7 +186,9 @@ export const BatchMapQueryRequest = {

fromJSON(object: any): BatchMapQueryRequest {
const message = createBaseBatchMapQueryRequest();
message.ids = (object.ids ?? []).map((e: any) => String(e));
if (Array.isArray(object?.ids)) {
message.ids = object.ids.map((e: any) => String(e));
}
return message;
},

Expand Down Expand Up @@ -306,8 +312,8 @@ export const BatchMapQueryResponse_EntitiesEntry = {

fromJSON(object: any): BatchMapQueryResponse_EntitiesEntry {
const message = createBaseBatchMapQueryResponse_EntitiesEntry();
message.key = object.key !== undefined && object.key !== null ? String(object.key) : '';
message.value = object.value !== undefined && object.value !== null ? Entity.fromJSON(object.value) : undefined;
message.key = isSet(object.key) ? String(object.key) : '';
message.value = isSet(object.value) ? Entity.fromJSON(object.value) : undefined;
return message;
},

Expand Down Expand Up @@ -360,7 +366,7 @@ export const GetOnlyMethodRequest = {

fromJSON(object: any): GetOnlyMethodRequest {
const message = createBaseGetOnlyMethodRequest();
message.id = object.id !== undefined && object.id !== null ? String(object.id) : '';
message.id = isSet(object.id) ? String(object.id) : '';
return message;
},

Expand Down Expand Up @@ -409,7 +415,7 @@ export const GetOnlyMethodResponse = {

fromJSON(object: any): GetOnlyMethodResponse {
const message = createBaseGetOnlyMethodResponse();
message.entity = object.entity !== undefined && object.entity !== null ? Entity.fromJSON(object.entity) : undefined;
message.entity = isSet(object.entity) ? Entity.fromJSON(object.entity) : undefined;
return message;
},

Expand Down Expand Up @@ -459,7 +465,7 @@ export const WriteMethodRequest = {

fromJSON(object: any): WriteMethodRequest {
const message = createBaseWriteMethodRequest();
message.id = object.id !== undefined && object.id !== null ? String(object.id) : '';
message.id = isSet(object.id) ? String(object.id) : '';
return message;
},

Expand Down Expand Up @@ -554,8 +560,8 @@ export const Entity = {

fromJSON(object: any): Entity {
const message = createBaseEntity();
message.id = object.id !== undefined && object.id !== null ? String(object.id) : '';
message.name = object.name !== undefined && object.name !== null ? String(object.name) : '';
message.id = isSet(object.id) ? String(object.id) : '';
message.name = isSet(object.name) ? String(object.name) : '';
return message;
},

Expand Down Expand Up @@ -644,3 +650,7 @@ if (util.Long !== Long) {
util.Long = Long as any;
configure();
}

function isSet(value: any): boolean {
return value !== null && value !== undefined;
}
6 changes: 5 additions & 1 deletion integration/bytes-as-base64/message.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ function createBaseMessage(): Message {
export const Message = {
fromJSON(object: any): Message {
const message = createBaseMessage();
message.data = object.data !== undefined && object.data !== null ? bytesFromBase64(object.data) : new Uint8Array();
message.data = isSet(object.data) ? bytesFromBase64(object.data) : new Uint8Array();
return message;
},

Expand Down Expand Up @@ -88,3 +88,7 @@ if (util.Long !== Long) {
util.Long = Long as any;
configure();
}

function isSet(value: any): boolean {
return value !== null && value !== undefined;
}
25 changes: 13 additions & 12 deletions integration/bytes-node/google/protobuf/wrappers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ export const DoubleValue = {

fromJSON(object: any): DoubleValue {
const message = createBaseDoubleValue();
message.value = object.value !== undefined && object.value !== null ? Number(object.value) : 0;
message.value = isSet(object.value) ? Number(object.value) : 0;
return message;
},

Expand Down Expand Up @@ -175,7 +175,7 @@ export const FloatValue = {

fromJSON(object: any): FloatValue {
const message = createBaseFloatValue();
message.value = object.value !== undefined && object.value !== null ? Number(object.value) : 0;
message.value = isSet(object.value) ? Number(object.value) : 0;
return message;
},

Expand Down Expand Up @@ -224,7 +224,7 @@ export const Int64Value = {

fromJSON(object: any): Int64Value {
const message = createBaseInt64Value();
message.value = object.value !== undefined && object.value !== null ? Number(object.value) : 0;
message.value = isSet(object.value) ? Number(object.value) : 0;
return message;
},

Expand Down Expand Up @@ -273,7 +273,7 @@ export const UInt64Value = {

fromJSON(object: any): UInt64Value {
const message = createBaseUInt64Value();
message.value = object.value !== undefined && object.value !== null ? Number(object.value) : 0;
message.value = isSet(object.value) ? Number(object.value) : 0;
return message;
},

Expand Down Expand Up @@ -322,7 +322,7 @@ export const Int32Value = {

fromJSON(object: any): Int32Value {
const message = createBaseInt32Value();
message.value = object.value !== undefined && object.value !== null ? Number(object.value) : 0;
message.value = isSet(object.value) ? Number(object.value) : 0;
return message;
},

Expand Down Expand Up @@ -371,7 +371,7 @@ export const UInt32Value = {

fromJSON(object: any): UInt32Value {
const message = createBaseUInt32Value();
message.value = object.value !== undefined && object.value !== null ? Number(object.value) : 0;
message.value = isSet(object.value) ? Number(object.value) : 0;
return message;
},

Expand Down Expand Up @@ -420,7 +420,7 @@ export const BoolValue = {

fromJSON(object: any): BoolValue {
const message = createBaseBoolValue();
message.value = object.value !== undefined && object.value !== null ? Boolean(object.value) : false;
message.value = isSet(object.value) ? Boolean(object.value) : false;
return message;
},

Expand Down Expand Up @@ -469,7 +469,7 @@ export const StringValue = {

fromJSON(object: any): StringValue {
const message = createBaseStringValue();
message.value = object.value !== undefined && object.value !== null ? String(object.value) : '';
message.value = isSet(object.value) ? String(object.value) : '';
return message;
},

Expand Down Expand Up @@ -518,10 +518,7 @@ export const BytesValue = {

fromJSON(object: any): BytesValue {
const message = createBaseBytesValue();
message.value =
object.value !== undefined && object.value !== null
? Buffer.from(bytesFromBase64(object.value))
: Buffer.alloc(0);
message.value = isSet(object.value) ? Buffer.from(bytesFromBase64(object.value)) : Buffer.alloc(0);
return message;
},

Expand Down Expand Up @@ -601,3 +598,7 @@ if (util.Long !== Long) {
util.Long = Long as any;
configure();
}

function isSet(value: any): boolean {
return value !== null && value !== undefined;
}
Loading