fix: use schemaName instead of methodName when building field resolve…

[dobrynin]

Hi! 👋

Firstly, thanks for your work on this project! 🙂

We are doing something more complicated, but I tried to boil it down to the essence of the issue below. Basically we have a mixin that adds very similar functionality that is parameterized by an argument name. We overwrite the schemaName appropriately by passing name to the field resolver, but when building the field resolver metadata, TypeGraphQL thinks that the subsequent field resolvers have already been added because it checks against the definition's methodName. I've run the test suite against the proposed change below,

Simplified breaking code:

const withMixin = <TObjectType extends ClassType<{ id: string }>>(
  ObjectType: TObjectType,
  BaseClass: ClassType,
  name: string
) => {
  @Resolver(() => ObjectType)
  class FoobarResolver extends BaseClass {
    @FieldResolver(() => String, { name })
    async foobar(
      @Arg(`${name}Id`, () => ID) foobarId: string
    ): Promise<any> {
       ...Common logic we want to add via the mixin that is parameterized by `name`...
    }
  }
  return FoobarResolver;
};

[MichalLytek]

@dobrynin Thanks for your contribution 🙌
In order to merge your PR, we need you to create a failing test case that reproduce the issue and which is then fixed by your changes in the codebase.
This way any other future refactor won't affect your use case.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (e28a028) 95.50% compared to head (a9fab9e) 95.50%.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #1284   +/-   ##
=======================================
  Coverage   95.50%   95.50%           
=======================================
  Files         113      113           
  Lines        1848     1848           
  Branches      364      364           
=======================================
  Hits         1765     1765           
  Misses         83       83           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[dobrynin]

@MichalLytek test added!

[dobrynin]

Hi @MichalLytek , is it possible to get an estimate for when this change can be merged + released?

[MichalLytek]

Please don't test metadata storage. It's implementation detail.
Your test cases should describe the real usage and real world assertion, so like "when I create this resolvers with this mixin, it should use this name, so that this field has this name and calls my field resolver properly".

[dobrynin]

Okay @MichalLytek , I've refactored the test to stop relying on metadata storage and moved the test to the resolvers.ts file. Please take a look when you have a chance!

[dobrynin]

Don't mean to nag you @MichalLytek, but please take a look at this one when you've had a chance. We'd like to stop using our fork of type-graphql :)

[itpropro]

I have a similar scenario currently, any news when this is integrated into the main branch @MichalLytek ?

[MichalLytek]

@itpropro Sorry to keep you waiting so long for this simple fix, I constantly forgot to merge after rebase 😛
Too much opened PRs and issues, too few time for all of that 😞

@dobrynin Can you check if your branch or PR still have enabled "allow edits from maintainers" setting?
I can't push changes anymore and I wanted to fix the rebase issues in order to merge this PR finally 🙏

[dobrynin]

@MichalLytek I don't see any option to do that, I think it may not be possible unfortunately because the fork lives under an organization: https://github.com/orgs/community/discussions/5634

[MichalLytek]

@dobrynin So could you rebase your branch and fix the lint issue (missing ! in test sample filed)? 🙏

[dobrynin]

@MichalLytek done

[MichalLytek]

@dobrynin Thank you very much for your contribution! ❤️