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

remove null from optional field type union #844

Closed
wants to merge 1 commit into from

Conversation

Alexendoo
Copy link

Messages with optional fields will now implement their corresponding interfaces (IMessage)

fixes #834
fixes #837

Messages with optional fields will now implement their corresponding
interfaces (IMessage)

fixes protobufjs#834
fixes protobufjs#837
Alexendoo added a commit to Alexendoo/clipboard-sync-chromium that referenced this pull request Jun 24, 2017
@djrenren
Copy link

Thanks! Hopefully this gets merged soon, cuz this bug has been giving me headaches.

@vaiwa
Copy link

vaiwa commented Jul 11, 2017

Thanks, How does it look with the merge?

@Alexendoo
Copy link
Author

@vaiwa instead of having optional fields typed as Field|undefined|null, they now match the interfaces in using Field|undefined. The result being you can use a Message as an IMessage rather than having to use type casting when using typescript

@dcodeIO
Copy link
Member

dcodeIO commented Jul 11, 2017

The reason that I'm hesitating here is that one can actually assign null without any consequences. All code explicitly checks for undefined or null. Some dependents might already be doing that, so this change might, or might not, break their code (apart from that it is probably using workarounds because this is still broken).

Ideally, null should be accepted for optional fields.

@Alexendoo
Copy link
Author

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 x|undefined is the canonical way to describe optional in typescript though

@djrenren
Copy link

To be fair, this change is only breaking under strictNullChecks: true

The real issue here is that without this change, the interfaces are unusable alongside the concrete types.

@dcodeIO
Copy link
Member

dcodeIO commented Jul 12, 2017

Does this commit fix anything in this regard?

@Conaclos
Copy link

Any news?

@Szpadel
Copy link

Szpadel commented Jul 25, 2017

@dcodeIO your commit makes things worse,
right now it broken our project that have strictNullChecks: true

@Szpadel
Copy link

Szpadel commented Jul 25, 2017

@Alexendoo could you create the same pull request to my fork?
I'm trying to create working protobufjs version that we need in production until official version will be fixed.

@Cixelyn
Copy link
Contributor

Cixelyn commented Jul 27, 2017

Wait, @Szpadel isn't ed7e2e7 sufficient?

Now all fields are correctly labeled as possibly null or undefined, which means that all the classes correctly implement the interface. This also more closely represents the JS API since both null and undefined are actually assignable.

I've submitted #881 which adds explicit inheritances to the typings.

@Szpadel
Copy link

Szpadel commented Jul 27, 2017

@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 strictNullChecks: true set in your project, then you cannot map fields directly to field?: SomeType, because this means that field is SomeType | undefined and protobuf field is SomeType | null | undefined and you are not allowing that null value here, of course you could refactor all your entities but this is just inconvenient especially when null value is impossible here.

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).
Therefore if types aren't compatible, then it will point errors (in this case only when strictNullChecks: true)

@dcodeIO
Copy link
Member

dcodeIO commented Nov 24, 2017

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?

@bourquep
Copy link

@Szpadel How did you end up working with/around this?

When you have strictNullChecks: true set in your project, then you cannot map fields directly to field?: SomeType, because this means that field is SomeType | undefined and protobuf field is SomeType | null | undefined and you are not allowing that null value here

@dudv
Copy link

dudv commented Apr 25, 2019

@dcodeIO The issue is still present in the latest version. Are there any plans to fix that?

@dcodeIO
Copy link
Member

dcodeIO commented Apr 25, 2019

Haven't looked at this for a while, but what I can tell is that the correct typing here is that actual instances of Message have OtherMessage | null fields while message-likes have OtherMessage | null | undefined (as in someField?: OtherMessage | null).

Seems that any attempts to resolve this didn't quite cut it because these either drop null or undefined from the equation in both cases, which isn't quite correct and leads to a "my fix is your bug" kind of situation.

This PR in particular changes

public repository?: (Package.IRepository|null);

to

public repository?: Package.IRepository;

on actual message instances, which is incorrect because instances don't have undefined but null there.

There might be smart ways to model this that I'm not aware of. Any ideas?

@mfedderly
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
10 participants