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

Extend a typedef in another file #228

Open
Xample opened this issue Jan 9, 2019 · 12 comments
Open

Extend a typedef in another file #228

Xample opened this issue Jan 9, 2019 · 12 comments
Assignees
Labels
Community 👨‍👧 Something initiated by a community Discussion 💬 Brainstorm about the idea Enhancement 🆕 New feature or request
Milestone

Comments

@Xample
Copy link

Xample commented Jan 9, 2019

Sometimes you want to add a property to whatever model but you want to keep all the logic (typedef and resolver) in a folder appart. This can be done using the ‘extend’ keyword in a typedef.
https://www.apollographql.com/docs/graphql-tools/generate-schema.html#extend-types Actually this is more a mixin than an extend. How could you achieve the same using type-graphql ?

@MichalLytek
Copy link
Owner

I am planing to add better modules (decoupling) support but this case is the one where I haven't found a good, typesafe API yet.

You can read this issue about the problem how to have module-scoped typedefs.

The only idea that I can think now is to have extend option in decorator to provide metadata for later merging in schema, rather that duplicating it:

// modules/user/user.ts
@ObjectType()
export default class User {
  @Field()
  email: string;

  @Field()
  password: string;
}

// modules/profile/user.ts
import BaseUser from "@modules/user";

@ObjectType({ extends: BaseUser })
class User extends BaseUser {
  @Field()
  profilePictureUrl: string;
}

But the problem with that approach is that it's hard to create a module that rely on two other modules that modify the User object type - TS has no support for multiple inheritance and multiple implements is a lot of declarations duplication.

@MichalLytek MichalLytek added Enhancement 🆕 New feature or request Community 👨‍👧 Something initiated by a community Discussion 💬 Brainstorm about the idea labels Jan 9, 2019
@Xample
Copy link
Author

Xample commented Jan 10, 2019

I personally thought about using a mixin https://www.typescriptlang.org/docs/handbook/mixins.html
But then, to what I see, we need to add glue code to explicitly merge 2 classes together. Using modules is "merged" by typescript itself but I also discourage keeping using modules with TS generally speaking.

@MichalLytek
Copy link
Owner

TS mixins are awful and (I hope) a deprecated pattern 😖

ES6 class mixins are the only solution:
https://youtu.be/j9dOdjBzARo?t=441

I plan adding to TypeGraphQL a type-safe port of mix(Foo).with(Bar) approach:
https://github.com/justinfagnani/mixwith.js#mixwith

@Xample
Copy link
Author

Xample commented Jan 18, 2019

Okay good so this way you can extend a class in another one, however, unlike an internal module (which would be merged by the language) you still need the parent class to know the subclasses to explicitly extend them.

@MichalLytek
Copy link
Owner

unlike an internal module (which would be merged by the language)

TS is not able to merge classes in namespaces (internal modules):

// a.ts
namespace MyApp {
  export class User {
    name!: string;
  }
}

// b.ts
namespace MyApp {
  export class User {
    age!: number;
  }
}

code_2019-01-18_11-04-56

To achieve this, TS would have to support partial classes:
microsoft/TypeScript#563

@Logic-Bits
Copy link

Logic-Bits commented Feb 25, 2019

Hi all,

TypeORM solved that already with "@ChildEntity()"

https://github.com/typeorm/typeorm/blob/master/docs/entity-inheritance.md

Would be awesome if Type-Graphql could implement it the same since alot is already inspired by the same stuff.


import { PrimaryGeneratedColumn, Column, Entity, ChildEntity } from "typeorm";
import { Field, ObjectType } from "type-graphql";
import { InvoiceDocument } from "../documents/invoice.model";

@ChildEntity()
@ObjectType({ description: "" })
export class CustomInvoiceFields extends InvoiceDocument {
    
    @Column()
    @Field({ nullable: true })
    CoolesExtraFeld: String;
}

TypeORM creates a Column in the "InvoiceDocument" Table here.

@MichalLytek
Copy link
Owner

MichalLytek commented Feb 25, 2019

@Logic-Bits So you propose the same solution as I showed earlier?

// modules/user/user.ts
@ObjectType()
export default class User {
  @Field()
  email: string;

  @Field()
  password: string;
}

// modules/profile/user.ts
import BaseUser from "@modules/user";

@ObjectType({ extends: true })
class User extends BaseUser {
  @Field()
  profilePictureUrl: string;
}

The extends: true means "don't create a new object type, instead add new fields to the parent class".

This solution is great but again - how then create a module that rely on two other modules that modify the User object type? Like User with profile picture and photos, we need them in a blog module.

@quentinus95
Copy link

@19majkel94 maybe it can be solved in code by using type unions? It's not perfectly safe but I don't see how it would impact any further improvement if we can find a better way to do type checking here.

@kaousheik
Copy link

What is the final solution ?

@dcurletti
Copy link

@kaousheik i believe the current "best" solution is to:

example:
one Graph type: User
two modules: User, Profile

// modules/user/user.ts
@ObjectType()
class User {
  @Field()
  email: string;

  // ideally this would be defined in the Profile module, but its currently not supported
  @Field()
  profilePictureUrl: string;
}

// modules/profile/user.resolver.ts
import User from "@modules/user";

@Resolver((of) => User)
class UserResolver {
  @ResolveField()
  profilePictureUrl() { ...logic }
}
  • define all fields on the singular entity class. In @MichalLytek 's example that would be: // modules/user/user.ts
  • import the singular entity class in various other modules, and define the resolver function in there.

This way you get some separation of concerns (you don't need to pass around your services), but you will need to pass around the entity classes.

For reference, the problem this issue is trying to solve is the equivalent of Nexus' extendType functionality (link).

@valerii15298
Copy link

+1 for this
Are there any better ways to do this for input types? Is there similar way for @inputType or should we just use native JavaScript extends to create new InputType based on existing InputType?

@MichalLytek
Copy link
Owner

@valerii15298 The purpose of extends in SDL is quite different that your @InputType case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Community 👨‍👧 Something initiated by a community Discussion 💬 Brainstorm about the idea Enhancement 🆕 New feature or request
Projects
None yet
Development

No branches or pull requests

8 participants
@Xample @Logic-Bits @dcurletti @quentinus95 @MichalLytek @kaousheik @valerii15298 and others