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 partial (subset) type matching #392

Closed
jbrantly opened this issue Aug 7, 2014 · 7 comments
Closed

Allow partial (subset) type matching #392

jbrantly opened this issue Aug 7, 2014 · 7 comments
Labels
Declined The issue was declined as something which matches the TypeScript vision Suggestion An idea for TypeScript Too Complex An issue which adding support for may be too complex for the value it adds

Comments

@jbrantly
Copy link

jbrantly commented Aug 7, 2014

Various libraries have a concept of applying partial updates to an object. For example:

http://backbonejs.org/#Model-set
http://facebook.github.io/react/docs/component-api.html#setstate

Take the following example (to roughly mimic Backbone)

interface Person {
    firstName: string
    lastName: string
    phoneNumber: string
}

var model = {
    set: function(data: Person) {}
}

model.set({
    phoneNumber: '405-555-1254'
})

In this case an error is thrown saying that properties are missing. One solution is to make all of the properties in the interface optional. However, that may not be desirable; I may actually want those properties to be required in other locations and only optional in this specific context. I would like to propose a feature where in a certain context a new type is created on-the-fly based on another type where all properties of the base type are optional. For example:

Person? => {
    firstName?: string
    lastName?: string
    phoneNumber?: string
}

and therefore the following would work:

var model = {
    set: function(data: Person?) {}
}

model.set({
    phoneNumber: '405-555-1254'
})

I think this would also work particularly well in conjunction with my other suggestion #391

set: function(data: Person?!) {}

Where zero or more properties from Person will match but ONLY properties from Person will match.

@danquirk
Copy link
Member

danquirk commented Aug 8, 2014

This seems strange to me. The way this API is modeled just seems incorrect. model.set simply does not take an object of type Person. The other overload for set clearly does not take a Person (set(propertyName: string, value: string)). And with your proposal now the API for Person is just incorrect. lastName is simply not optional as far as your model is concerned, it's required, but you want it optional just to accommodate for this set API. Now you'll have a bunch of Person? objects that are not assignable to Person objects and any time they need to mix you'll have to add casts. And you need to add these duplicate type definitions for every single type related to a model? I think you'd just be transferring the pain from one place (set) to somewhere else while requiring new syntax.

@jbrantly
Copy link
Author

jbrantly commented Aug 8, 2014

Hi Dan,

Thanks for the feedback. I feel like there may be some confusion on what this suggestion is trying to accomplish. If I could take your points one by one.

model.set simply does not take an object of type Person.

That's absolutely correct. According to the Backbone documentation, "a hash of attributes (one or many) on the model", which precisely aligns with the intent of this suggestion. model.set is not supposed to take an entire Person, it's supposed to take a subset of Person. For instance, say the user types the lastName into a field and I now want to update the model, I don't want to have to construct an entire Person to update the model, just a hash with the single property. And ideally I'd like to be able to do this in type-checked way.

And with your proposal now the API for Person is just incorrect. lastName is simply not optional as far as your model is concerned, it's required, but you want it optional just to accommodate for this set API.

I don't think in my proposal the API for Person is incorrect. If I'm constructing a new instance of Person then I want lastName to be required. If I'm doing some sort of partial update to Person using a merge, then I want a subset of Person. I feel that this concept is used fairly liberally in the JS world. I've given two examples (Backbone and React). I think option bags could also benefit from this suggestion.

Now you'll have a bunch of Person? objects that are not assignable to Person objects and any time they need to mix you'll have to add casts.

In no case that I've described would I be assigning Person? objects to Person. I would be merging them though. And I also assume that Person would be assignable to Person? so no issues there.

And you need to add these duplicate type definitions for every single type related to a model?

I'm not sure I understand. This proposal is to avoid adding duplicate type definitions. Consider:

class Model<T> {
    ...
    set(hash: T?): any;
    ...
}

Now, as a user of Backbone, I can simply define Person and the API will just magically work with something like:

var model = new Model<Person>();
model.set({lastName: 'Smith'}); // this is type-checked to be Person?

I think you'd just be transferring the pain from one place (set) to somewhere else while requiring new syntax.

To be clear: the pain is not in the implementation of set. The current pain is that as a caller of set, I lose my type-checking. The two current options to update the model are to provide a subset of the interface or to provide a string which is the property name. In both cases I lose compile-time checks to account for refactoring and typos. This proposal is to help solve the first option and #394 is to help solve the second.

Understood if this remains closed but wanted to make sure the proposal was well understood.

@jbrantly
Copy link
Author

jbrantly commented Aug 9, 2014

A small correction: regarding compile-time checks, I realize this proposal by itself won't fix that. However, it does make it so that I do not need to make every property on my models optional in order to support partial updates as I currently do.

@richardhuaaa
Copy link

+1

@realyze
Copy link

realyze commented Sep 6, 2015

+1 This would make mongo projections in TS much safer.

@BowserKingKoopa
Copy link

This should be reopened. This is a thing.

@jbrantly
Copy link
Author

@BowserKingKoopa I think this is essentially reopened in #4889. I would track there.

@microsoft microsoft locked and limited conversation to collaborators Jun 18, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Declined The issue was declined as something which matches the TypeScript vision Suggestion An idea for TypeScript Too Complex An issue which adding support for may be too complex for the value it adds
Projects
None yet
Development

No branches or pull requests

5 participants