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

Add JustProps and JustMethods types #4

Closed
wants to merge 6 commits into from

Conversation

tonivj5
Copy link

@tonivj5 tonivj5 commented Mar 13, 2019

No description provided.

@sindresorhus
Copy link
Owner

Would maybe ExtractProperties and ExtractMethods be clearer names for these?

@sindresorhus
Copy link
Owner

Can you share some real-life situations where these have been useful? The included examples are good for showing what they do, but not when you would need them.

index.d.ts Outdated Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
index.d.ts Show resolved Hide resolved
@sindresorhus sindresorhus changed the title feat: add JustProps and JustMethods types Add JustProps and JustMethods types Mar 13, 2019
sindresorhus and others added 2 commits March 13, 2019 22:49
@tonivj5
Copy link
Author

tonivj5 commented Mar 13, 2019

I use JustProps a lot when I receive an object from an API and that has the form of the model, but it isn't an instance.

class User {
  name: string;
  age: number;

  doSomething() { ... }
}

function getUser(id: number): Observable<User> {
  return this.http.get<JustProps<User>>(...uri...).pipe(map(user => new User(user));
}

Or when I manage only object properties (for example in a form or updating the properties of a database model)

function fillForm(user: JustProps<User>): void {
 ...
}

function insert(user: JustProps<User>): Promise<User> {
    return this.userRepository.insert(user);
}

In these examples I don't want the user's methods, only need its properties.

I think these real-life examples are clear enough, should I include it as examples in the code?

I haven't used JustMethods yet, I did it because once JustProps was done, defining JustMethods just meant changing a word 😆

Regarding how to call it, I still think JustProps is better, it's more readable from my point of view

@EdwardDrapkin
Copy link

EdwardDrapkin commented Mar 14, 2019

I don't think this is a good idea. I don't see any opportunity to use this in a way that doesn't violate some key tenets of good type modeling, particularly the single responsibility principle. What you've done (with your example), is take a single type (User) and give it two responsibilities: representing user data and representing actions on that data. In a context where you would never have data decoupled from its mutators, that's obviously fine, but that's not the case in your example. You should be using interface User and class UserDAO implements User or something similar.

The only thing this type does is allow developers to misuse too-large types rather than refine them into a more granular type hierarchy. I remain completely unconvinced that it's possible to use this in a way that doesn't violate SRP. This is a classic footgun.

@sindresorhus
Copy link
Owner

@xxxTonixxx I just added some contribution guidelines. Would you be able to review them? I'm also interested in feedback if anything there is unclear or could be improved.

https://github.com/sindresorhus/type-fest/blob/master/.github/contributing.md

@sindresorhus
Copy link
Owner

@EdwardDrapkin I agree with your principle, but it is a common pattern in many frameworks:

@sindresorhus
Copy link
Owner

@xxxTonixxx Do you have any other use-cases for these types other than the ones mentioned already?

@sindresorhus
Copy link
Owner

@CvX @BendingBender Thoughts?

@BendingBender
Copy link
Contributor

Not sure whether it's actually useful, I've never needed types like this.

@EdwardDrapkin
Copy link

@EdwardDrapkin I agree with your principle, but it is a common pattern in many frameworks:

* Django: https://docs.djangoproject.com/en/2.1/topics/db/models/#model-methods

* Rails: https://guides.rubyonrails.org/active_model_basics.html#attribute-methods

They have LiveRecords, which is fine. The issue is that he's trying to use the same type to represent just a serialization of data (e.g. from an API) and the LiveRecord itself.

@tonivj5
Copy link
Author

tonivj5 commented Mar 15, 2019

I use it to avoid creating redundant types, why defining an interface whose purpose to maintein only properties? If my model only changes beetwen my 'UserDAO' and 'User' in its methods.... That's its use.

If my model and its interface would have different properties or use-cases, I understand to have both.

I honestly see this redundant...

interface User {
  name: string;
  age: number;
  address: string;
  a1: string;
  a2: string;
  a3: string;
  a4: string;
  a5: string;
  a6: string;
  ....
}
class UserDAO implements User {
  name: string;
  age: number;
  address: string;
  a1: string;
  a2: string;
  a3: string;
  a4: string;
  a5: string;
  a6: string;
  ...

  doSomething() { ... }
  doSomething2() { ... }
  doSomething3() { ... }
}

When I can just use JustProps<User> when it's appropriate.
In my case it saves me code and time, because it fits with how it's my architecture. I understand that you find it as an antipattern, it depends on your type modeling.

Another use-case example where I use it's to type constructor

class User {
  name: string;
  age: number;
  address: string;
  a1: string;
  a2: string;
  a3: string;
  a4: string;
  a5: string;
  a6: string;
  ...

  constructor(user: JustProps<User>) {...}

  doSomething() { ... }
  doSomething2() { ... }
  doSomething3() { ... }
}

// it allows me to use it in this way
const user = new User({ name: 'aaa', age: 32, ... });

@sindresorhus
Copy link
Owner

Thanks for suggesting this type. However, I have decided to pass on it for reasons outlined in the discussion here.

@rfgamaral
Copy link

I'd like to make a case for JustProps reconsideration. Here's my use case:

I'm doing a GraphQL API which calls a REST API. I'm also using type-graphql which means that I have a Class for each GraphQL Type. Take for instance this Launch class (which will end up as the Launch GraphQL Type):

@ObjectType()
class Launch {
    @Field(() => ID)
    id!: string;

    @Field({ nullable: true })
    site?: string;

    @Field(() => Mission, { nullable: true })
    mission!: Mission;

    @Field(() => Rocket, { nullable: true })
    rocket!: Rocket;
}

When I fetch data from my REST API, the model is completely different and I need to map it to the GraphQL Type to properly return it within my GraphQL API. For instance, this is the method which makes the REST request:

async fetchOne(id: string): Promise<Launch> {
    const response = await this.get<RESTLaunch>(`launches/${id}`);
    return launchReducer(response);
}

And this is the launchReducer which maps a RESTLaunch type (manually defined by me) to Launch (the GraphQL Type):

function launchReducer(launch: RESTLaunch): Launch {
    return new Launch({
        id: String(launch.flight_number || 0),
        site: launch.launch_site?.site_name,
        mission: {
            name: launch.mission_name,
            smallPatchUrl: launch.links.mission_patch_small,
            largePatchUrl: launch.links.mission_patch,
        },
        rocket: {
            id: launch.rocket.rocket_id,
        },
    });
}

As you can see, I'm passing a data structure that correctly maps the Launch class fields. Now I need a type for the single constructor argument to initialize my class properties with the data structure I'm passing to the constructor, ignoring any methods (although I don't have one in my Launch class example, I could have something like a getMissionName(), for instance).

This is where the JustProps type would come in handy:

constructor(data: JustProps<Launch>) {
    Object.assign(this, data);
}

This is my case for including JustProps (not sure about the name, maybe ExtractProps or OnlyProps would be a better fit) in type-fest, not sure if it's enough for your to reconsider.


As a workaround, I believe I can achieve the same result by leveraging the current type definitions in type-fest, like this:

type JustProps<Base> = ConditionalExcept<Base, Function>;

@alexrififi
Copy link

Useful types. It's a pity they didn't accept 🥺

@ayloncarrijo
Copy link

This type would be very useful until TypeScript offers a better way to handle named properties. microsoft/TypeScript#29526

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.

7 participants