type-graphql icon indicating copy to clipboard operation
type-graphql copied to clipboard

Field accessors

Open arcanis opened this issue 3 years ago • 7 comments

Is your feature request related to a problem? Please describe. MikroORM has a feature called References - basically, fields contain a wrapper that itself contain the true relation entity. Those wrappers are an implementation detail, and thus shouldn't be exposed through the GraphQL API.

Describe the solution you'd like I'd like my Field decorator to support defining a custom data accessor (by default an identity function):

@ObjectType()
class Book {
  @Field(() => User, {get: author => author.load()})
  author!: Reference<User>;
}

Describe alternatives you've considered I'm currently doing the following, which requires a somewhat heavy boilerplate since I have to create "unneeded" class properties and thus loose coupling:

@ObjectType()
class Book {
  authorRef!: Reference<User>;

  @Field(() => User)
  get author() {
    return this.authorRef.load();
  }
}

arcanis avatar Dec 09 '20 23:12 arcanis

Another option is to make a decorator. It's more verbose than the first alternative, but you only pay the price once:

function ReferenceField(returnTypeFunction: any, opts?: FieldOptions) {
  return (target: any, propertyKey: string) => {
    const unwrappedField = `unwrapped_${propertyKey}`;
    Object.defineProperty(target, unwrappedField, {
      get () {
        return this[propertyKey].load();
      },
    });
    Field(returnTypeFunction, {...opts, name: propertyKey})(target, unwrappedField);
  };
}

arcanis avatar Dec 10 '20 00:12 arcanis

@Field(() => User, {get: author => author.load()})

This makes no sense if you can just use class geters 😉

Another option is to make a decorator.

This sounds better. It's non-repetitive, declarative and easy to reason about when placed on the field:

@ObjectType()
class Book {
  @ReferenceField(() => User)
  author!: Reference<User>;
}

Just check the d.ts files for proper typings for ReferenceField params, as any smells 😅

MichalLytek avatar Dec 10 '20 18:12 MichalLytek

Is your issue resolved now or maybe I should think about making such generated field decorators easier?

I wonder if you could use field middleware to call .load() method on the returned value 🤔

MichalLytek avatar Dec 10 '20 18:12 MichalLytek

Just check the d.ts files for proper typings for ReferenceField params, as any smells 😅

Yeah I agree, but parameter forwarding is annoying with TS - especially since ReturnTypeFunc isn't exported as part of the public interface 🤔

Is your issue resolved now or maybe I should think about making such generated field decorators easier?

I think it could be a bit easier - that orm entities and GraphQL entities follow slightly different plumbing / wrapping doesn't seem that uncommon, so personally I'd suggest to at least document what to do when this case happens. I still think the getter would make sense, though - if only because writing custom decorators would then become straightforward:

const ReferenceField = (returnTypeFunction, opts) => Field(returnTypeFunction, {...opts, get: ref => ref.load()});

It doesn't have to be get, other option names would work: map, transform, unwrap, ...

arcanis avatar Dec 10 '20 18:12 arcanis

that orm entities and GraphQL entities follow slightly different plumbing / wrapping doesn't seem that uncommon

But it's really library-specific and TypeGraphQL's goal is to be library-agnostic - you can easily plug-in your own validation mechanism (library).

So instead of hardcoding ref.load() I would like to introduce concept of wrapped fields:

const ReferenceField = createWrapperFieldDecorator<Reference<unknown>>(property => property.load());

@ObjectType()
class Book {
  @ReferenceField(() => User)
  author!: Reference<User>;
}

So it would replace the default return root[property] field resolver with return wrapperFn(root[property]).

Middlewares can do exactly the same but they are more universal and have a negative performance overhead. They are recommended for calling entity.toObject() like in the typegoose example in the repo.

MichalLytek avatar Dec 10 '20 21:12 MichalLytek

Oh for sure, hardcoding load() isn't the right way - the snippet I shared:

const ReferenceField = (returnTypeFunction, opts) => Field(returnTypeFunction, {...opts, get: ref => ref.load()});

wasn't meant to be something in type-orm except for the get option support (the user would then be responsible for implementing the wrapper). In essence, I think your proposal is doing pretty much the same, but with a different API. Would be fine by me 👍

arcanis avatar Dec 10 '20 21:12 arcanis

Hello,

The above workaround (using a getter ) doesn't work for me, getting

Cannot set property xxxof #<Modelxxx> which has only a getter"

As a workaround I have to create a setter which is quite ugly but works.

The culprit seem to be here:

type-graphql\\dist\\helpers\\types.js:78:19 which creates a new Model which will throw an error when trying to set the getter field.

gotexis avatar Apr 17 '22 03:04 gotexis