Feature presence fields 2

[tomasz-szypenbejl-td]

This pull request is a new attempt to add field presence/absence indicators, and to optimize the serialization code. It mostly contains @Yurzel's commits from #136.
Additionally this pull request alters the generated toObject() methods so that the returned objects contain default values.

I would still like to write some additional tests to make sure that serialization really works as expected with regard to skipping fields that are OK to be skipped (i.e. unset fields for proto2 and defaults for proto3). Thus I am creating this as a draft for now. I should be able to complete work on this on Friday (2022-06-17), but it may take more time if some unexpected difficulties arise.

[tomasz-szypenbejl-td]

Additional tests have been added, and I made a few further corrections to the createSerialize() function.

@thesayyn, please review the pull request when you have some time. Should a rework be needed, I will probably be able to do the rework next week.

[thesayyn]

It looks way better now. but I'd prefer to have presence fields as getter instead of method.

Also, why do we need clear methods? is it part of the spec? If not, how is it different from setting the field to undefined?

[thesayyn]

I wasn't aware that we needed this. How's this any different than setting patch = undefined?

[tomasz-szypenbejl-td]

This is an internal project file, and the whole project seems to be compiling from typescript to javascript without strictNullChecks option of tsc enabled. So I guess the clear_patch() method is really unnecessary here.

On the other hand, for users of protoc-gen-ts who have the strictNullChecks option enabled in their projects the existence of the clear_property() methods makes sense. With strictNullChecks enabled the tsc compiler will not allow the assignment of undefined, like patch = undefined in this example, simply because the setter accepts a value of some specific type other than undefined (a number in this example.)

[tomasz-szypenbejl-td]

It looks way better now. but I'd prefer to have presence fields as getter instead of method.

I changed the has_ methods to getters. I agree getters are better. Not only are they more natural, but also with getter you will never forget to type the () as you would have to do with a function call.

Forgetting the () in a zero-argument function call is a mistake that the compiler might not catch. I learned that the hard way - I really forgot the () last Friday in one line of code, and found the problem only because some tests did not pass. With getters instead of functions nobody will be able to make such a mistake anymore.

Also, why do we need clear methods? is it part of the spec? If not, how is it different from setting the field to undefined?

I think the clear_property() methods have been added by @Yurzel because Google's solution generates such methods. I think keeping the clear methods actually makes sense. The setters are not really accepting undefined values per their signature, e.g.:

set patch(value: number) {
    pb_1.Message.setField(this, 3, value);
}

so I guess to really make the setter accept undefined as a value, we would probably have to change the setter to

set patch(value: number | undefined) {
    pb_1.Message.setField(this, 3, value);
}

As mentioned in an earlier comment, that would only be important for protoc-gen-ts users who have "strictNullChecks": true in their project's tsconfig.json.

I kept the clear methods for now, but I can remove them (and change setters signatures) if you insist.

[thesayyn]

so I guess to really make the setter accept undefined as a value, we would probably have to change the setter to

That's actually what issue #80 is about. if a field is optional, then you should be able to set it to undefined. That's why i am opposed to adding the clear methods. Maybe we keep it out of this PR and handle it with another PR?

[thesayyn]

Amazing work so far @tomasz-szypenbejl-td

[tomasz-szypenbejl-td]

so I guess to really make the setter accept undefined as a value, we would probably have to change the setter to

That's actually what issue #80 is about. if a field is optional, then you should be able to set it to undefined. That's why i am opposed to adding the clear methods. Maybe we keep it out of this PR and handle it with another PR?

Alright, I will remove the clear methods so that this pull request can be merged.
I understand that I can leave the setters unchanged for now because a separate pull request (to address the issue #80) will eventually take care of the setters.

Amazing work so far @tomasz-szypenbejl-td

Thank you.

[tomasz-szypenbejl-td]

I removed the clear methods. @thesayyn please take a look - if there is anything that still needs to be fixed, just let me know.

[thesayyn]

awesome. just @deprecated annotations are missing for has getters are belongs to a deprecated field.

[thesayyn]

Nice job!

[tomasz-szypenbejl-td]

Nice job!

Thank you. I have to give credit to @Yurzel. He did most of the hard work writing the code using ts.factory. I just modified and shuffled around that code a bit, and I am really happy that I did not have to write it all from scratch myself (it would have taken me a plenty of time).

Thank you for approving and merging this pull request :)

[Yurzel]

Nice job!

Thank you. I have to give credit to @Yurzel. He did most of the hard work writing the code using ts.factory. I just modified and shuffled around that code a bit, and I am really happy that I did not have to write it all from scratch myself (it would have taken me a plenty of time).

Thank you for approving and merging this pull request :)

Ah I probably should have mentioned this tool:
https://ts-ast-viewer.com/

[td-krzysiek]

@thesayyn any chance for the release, please? Will be much appreciated!