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

Allow all keys to be optional in the JsonObject type #65

Merged
merged 3 commits into from Feb 14, 2020

Conversation

yxliang01
Copy link
Contributor

By right, when an object has certain keys optional, the object is still a valid JsonObject. So, it makes more sense to allow all keys to be optional. Indeed, an JsonObject doesn't always have values for all string indexes.

This resolves #64 .

@yxliang01 yxliang01 changed the title fix(JsonObject): allow all keys to be optional [WIP] fix(JsonObject): allow all keys to be optional Oct 8, 2019
@yxliang01 yxliang01 changed the title [WIP] fix(JsonObject): allow all keys to be optional fix(JsonObject): allow all keys to be optional Oct 8, 2019
@sindresorhus
Copy link
Owner

Can you add some type tests?

@yxliang01
Copy link
Contributor Author

@sindresorhus Yup I can. But, before that, I need to know whether these changes look good :) Otherwise, the tests will be useless :P

@sindresorhus
Copy link
Owner

@SamVerschueren Is this what you had in mind?

@SamVerschueren
Copy link
Contributor

Just wondering, but key in string doesn't look like the same as a key which is of type string.

@sindresorhus
Copy link
Owner

@yxliang01 Why did you change it to Key in string?

@yxliang01
Copy link
Contributor Author

yxliang01 commented Nov 29, 2019

@sindresorhus It was simply that [k: string]?: value is not valid for its syntax. This can actually be written as [k: string]: value | undefined, just that I am not aware of semantic difference between [k: string]: value | undefined and [k in string]?: value for the case of accepting any key of type string. So, it might be more concise to modify it to [k in string]?: value .

@sindresorhus
Copy link
Owner

I'm not aware of the difference either. One problem with either of these solutions is that it will allow:

interface Unicorn extends JsonObject {
	rainbow: undefined;
}

@yxliang01
Copy link
Contributor Author

yxliang01 commented Nov 30, 2019

So, the intended behavior is that all keys can be optional while it is a must that we can't make the value undefined assignable to all defined keys. Although in the underlying JavaScript compiled code, these two has only very subtle semantic difference, the current unintended semantic will enlarge the room for errors.

The technical difficulty to achieve the intended semantic is that the TypeScript type system doesn't differentiate the following cases:

  1. Key k is defined with value undefined
  2. Key k is undefined

Note that these two cases are different from the JavaScript perspective.

Thus, it might be impossible to achieve our goal with current TypeScript type system?

@sindresorhus
Copy link
Owner

@resynth1943 Any thoughts on this?

@resynth1943
Copy link
Contributor

I'll take a look and see what I think.

@papb
Copy link
Contributor

papb commented Feb 13, 2020

Related: microsoft/TypeScript#13195

@resynth1943
Copy link
Contributor

Apologies for the late reply.

I don't really see any major problems with this regarding serialized and de-serialized JSON types, as keys could be optional in that context. LGTM.

@resynth1943
Copy link
Contributor

resynth1943 commented Feb 14, 2020

@yxliang01 Could you please follow the instructions in this document?
Contributing

@sindresorhus sindresorhus changed the title fix(JsonObject): allow all keys to be optional Allow all keys to be optional in the JsonObject type Feb 14, 2020
@sindresorhus sindresorhus merged commit 6c105fb into sindresorhus:master Feb 14, 2020
@yxliang01 yxliang01 deleted the patch-1 branch February 14, 2020 19:54
@yxliang01
Copy link
Contributor Author

Finally merged after created for 4 months 🎉

@resynth1943
Copy link
Contributor

Yay!

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

Successfully merging this pull request may close these issues.

JsonObject doesn't allow optionals
5 participants