-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
remove null from optional field type union #844
Conversation
Messages with optional fields will now implement their corresponding interfaces (IMessage) fixes protobufjs#834 fixes protobufjs#837
Thanks! Hopefully this gets merged soon, cuz this bug has been giving me headaches. |
Thanks, How does it look with the merge? |
@vaiwa instead of having optional fields typed as |
The reason that I'm hesitating here is that one can actually assign Ideally, |
Yeah this is a breaking change as far as type definitions go, but the benefits far outweigh the negatives (one can simply use undefined where they previously used null if migration is neccessary). Just |
To be fair, this change is only breaking under The real issue here is that without this change, the interfaces are unusable alongside the concrete types. |
Does this commit fix anything in this regard? |
Any news? |
@dcodeIO your commit makes things worse, |
@Alexendoo could you create the same pull request to my fork? |
Wait, @Szpadel isn't ed7e2e7 sufficient? Now all fields are correctly labeled as possibly I've submitted #881 which adds explicit inheritances to the typings. |
@Cixelyn AFAIK protobuf is encoding empty repetable field as missing also all empty fields are just missing ones, therefore it is impossible to have null value here. When you have Edit: sorry I was thinking you are referring to ed7e2e7, but it is basically the same case, because you don't need to explicit implement any interface, it implements it automatically when interfaces matches (implements is only to ask typescript to validate your class to point errors in the right place). |
Does this issue still exist in master? There have been a couple merges that might, or might not, prevent this. Otherwise, do you have an updated example causing the issue? |
@Szpadel How did you end up working with/around this?
|
@dcodeIO The issue is still present in the latest version. Are there any plans to fix that? |
Haven't looked at this for a while, but what I can tell is that the correct typing here is that actual instances of Seems that any attempts to resolve this didn't quite cut it because these either drop This PR in particular changes
to
on actual message instances, which is incorrect because instances don't have There might be smart ways to model this that I'm not aware of. Any ideas? |
Also related to #1171 On my own project, I stripped out the generated interfaces entirely (between pbjs and pbts), because I am only using the decode methods and everything will be of the class type not the plain javascript object type. Referencing the interface types also causes issues if you extend the generated classes with additional methods. Would a separate PR be accepted to pbjs to skip generating the interface jsdoc comments? That gives people the flexibility to use the classes only and prevent what seems to be a common pitfall with protobufjs and typescript. |
Messages with optional fields will now implement their corresponding interfaces (IMessage)
fixes #834
fixes #837