Added basic implementation for #124

[wesselvdv]

Based on the issue #124, I added some very basic tests let me know if more are desired.

[wesselvdv]

Why is the ci failing? All I can see is some comment about a time out, I can't reproduce this locally.

[MichalLytek]

Why is the ci failing?

I thought that's new jest setup issue but looks like it's travis fault. I've fixed it on the branch 78990ab, wait for the merge and then rebase your PR.

[MichalLytek]

Sorry, but that's not what we were discussing in #124.
The purpose of @Metadata decorator is to emit fields in GraphQL type config, just like complexity does, not to place metadata key. It should also allow for multiple @Metadata calls to compose your own decorators like @Complexity.
If you want, we can iterate on this PR 😉

[wesselvdv]

I’m not sure I follow? I need to store the custom object somewhere where the chance it conflicts with other parameters is the lowest. (e.g. to prevent accidental key/paramter collisions.) I am however using object destructuring to add the entire metadata object to the type config on creation. Which is what I believe you also suggested no?

[MichalLytek]

Why you can't just use it then like e.g.:

export cost nonConflictingSymbol = Symbol("wesselvdv");

@ObjectType
class MyType {
  @Metadata({
    superRareFieldKey: myData,
    [nonConflictingSymbol]: myOtherData,
  })
  @Field()
  field: FieldType;
}

[wesselvdv]

That could definitely work, but how to get it on the type config then? For example for joinmonster we really need specific parameters to exists in the typeconfig, not in the metadata storage.

I envisioned that any decorator or middleware can now just populate the metadata property and be assured that it’ll appear in the type config. This would allow specific decoratora for specialized properties that need to be available for joinmonster or other libs.

Come to think of it, I am not sure if there are other libs that work in the same fashion. Joinmonster does, even though I am quite sure one can rewrite that to work more like the query complexity thingy. That would definitely ensure a more proper abstraction between the library and graphql.

[MichalLytek]

Runtime example

// type metadata definition
@ObjectType
class MyType {
  @Metadata({
    complexity: 5,
  })
  @Metadata({
    isTopSecret: true,
  })
  @Field()
  field: FieldType;
}

// metadata storage
metadata: [
  {
    complexity: 5,
  },
  {
    isTopSecret: true,
  },
],

// GraphQLObjectType
fields: () => {
  // ...
  complexity: 5,
  isTopSecret: true,
},

I can't explain that simpler 😆

[wesselvdv]

I believe we’re talking about different things, in your approach could I theoretically get that custom metadata from the schema definition contained in the resolveinfo of a resolver? (e.g. using the @Info decorator?)

[MichalLytek]

But you are actually doing the same 😆
https://github.com/19majkel94/type-graphql/blob/85b3f47472f67ed27067f459ff00bae2f27638b2/src/schema/schema-generator.ts#L243-L245

It just should be ...field.metadata.reduce((obj, metadata) => ({...obj, ...metadata}), {}) and for every type, not only object type. And testing metadata storage without checking the actual emitting in schema makes no sense.

[wesselvdv]

I can agree with the testing part. Any pointers you can give me there, other than loose statements and bits of code?

I took the liberty to quote your comment from #124:

@wesselvdv
Not yet but I think it's easy to implement even by beginners 😉

API usage - @Metadata:

@ObjectType()
class MyObjectType {
  @Metadata({ isUseful: false })
  @Field()
  myAnnotatedField() {
    return "12"
  }
}

Steps to do:

• create Metadata decorator that accepts object
• modify field/query/mutation/subscription/input metadata interfaces in storage
• create a method in metadata storage that will be called by Metadata to place the metadata object in storage
• use that metadata object during schema construction in all object types, input, handlers, etc.:

fieldsMap[field.schemaName] = {
  // ...
  deprecationReason: field.deprecationReason,
  ...(field.metadata || {}),
};

• of course test this feature well and create documentation for it 😉

I believe that what I did is what you tried to describe there? Difference is that I only created it for a field and an object type.

From what I understand from you now, you would rather collect the metadata in an array, and then consolidate them in the corresponding type on schema creation? (eg. the reduce action with double destructuring)

The purpose of @metadata decorator is to emit fields in GraphQL type config, just like complexity
does, not to place metadata key.

This comment I really don't understand, I need to store the metadata somewhere? I can't just slap it on the corresponding type its metadata storage, I believe that would get messy and it'd be easy to start messing with the internals of type-graphql unwittingly. Besides you yourself keep referring to a metadata property on a given type its metadata storage.

[MichalLytek]

I see that we're not able to make up about this 😄
I have no time to deliberate on this feature any longer, looks like you have to wait until I implement it in my own way, sorry 😉

[wesselvdv]

Alright, your call...

[MichalLytek]

Sorry, but I've wrote it a few times and I've even showed an example that it should allow for multiple decorators, so you just have to store an array in metadata storage and flatten it in schema generator. Quoting my old posts about old designs won't improve this PR magically 😜 Also while going stick to the #124 guideline you've forgot about "in all object types, input, handlers, etc.".

And by:

is to emit fields in GraphQL type config, just like complexity does, not to place metadata key

I was referring to the tests when you only check the metadata storage, not the e2e emitting properties in object type instance 😉

Is it just a little bit more clear now?
And sorry but I have no time to guideline every contributor and make a checklists of all steps for a simple PR. We can iterate on this PR if you know what and how to do, otherwise it's waste of time that I can use just by implementing it.

[wesselvdv]

Sorry, but I've wrote it a few times and I've even showed an example that it should allow for multiple decorators, so you just have to store an array in metadata storage and flatten it in schema generator.

Explaining something badly in a couple of comments doesn't magically make it a good explanation. You leave out a lot of stuff that happens in between your small examples, and this stuff might not have to be mentioned to you or any long time contributor, but for someone like me who hasn't memorised your source code it can be hard to follow.

Your "example" is a snippet where metadata is suddenly shown in the fields object instead of being properties of a field.

fields: () => {
  // ...
  complexity: 5,
  isTopSecret: true
}

vs

fields: () => {
  // ...
  field1: {
      complexity: 5,
      isTopSecret: true,
  }
}

I assume that the latter is what you expect? That's what I would expect when adding metadata to a field type.

Quoting my old posts about old designs won't improve this PR magically 😜 Also while going stick to the #124 guideline you've forgot about "in all object types, input, handlers, etc.".

Quoting your old post was just to show you why I chose the approach I did, and I already said that I didn't implement it for other types. The reason for that is twofold, one for the fact that I didn't need it for joinmonster, and the other for reducing the amount of stuff I would have to refactor should it have to work differently.

And by:

is to emit fields in GraphQL type config, just like complexity does, not to place metadata key

I was referring to the tests when you only check the metadata storage, not the e2e emitting properties in object type instance 😉

Alright, that makes sense.

Is it just a little bit more clear now?
And sorry but I have no time to guideline every contributor and make a checklists of all steps for a simple PR. We can iterate on this PR if you know what and how to do, otherwise it's waste of time that I can use just by implementing it.

It's definitely more clear now, but what might a "simple" PR for you or someone who's been working on this project for a while, can be a different matter for someone who isn't. If you're not willing or capable to explain yourself more thoroughly when asked, then you shouldn't let the community assist you. You can't expect people to think exactly the same way you do, we're all trying our best and we're not here to annoy you. So for the sake of helping contributors a set of guidelines might be super useful. So instead of having this discussion with me or any other type-graphql noob, you'd only have to refer to your guidelines. Makes everyone's lives easier. 😉

I was just trying to (partially) implement it the way you dictated in #124, and while I am definitely willing to accept criticism, you should know that the way you communicate has a hint of a condescending tone. Comments such as "I think it's easy to implement even by beginners 😉", "Quoting my old posts about old designs won't improve this PR magically 😜" or "I can't explain that simpler 😆" won't motivate people to assist you. I am not sure if this is what you intend, but I don't think it's the best way to get people to help you.

[MichalLytek]

Sorry for that. I'm a bit irritated because of my master's thesis so I may turn into a diva. You should better give me a snickers! 😄
https://www.youtube.com/watch?v=GXCVAGio4uQ

I will create for sure some generic guidelines and checklist for any PR maker, e.g. I should turn on a pre-commit hook for Prettier. That will save my time as I always have to remember contributors about documentation and proper tests.

And I don't want to be rude but I have to keep a balance between helping/assisting and coding. I would rather create a ready-to-merge PR in 2 hours than explaining all steps for newbies, pointing all spots in code that should be changed, reviewing each PR iteration and merging it after a week. That's why I try to use minimal amount of time to describe the implementation details which as we see is sometimes too few to have a clear vision.

Feel free to continue working and experiment on this feature or use your fork until I get through to this issue as there are other issues with higher priority 😞

Over and out 😃

[wesselvdv]

Alright, awesome! I'll reopen this one when I have some time to rework it to what you want.