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

Multiple Args include all arguments #570

Closed
backbone87 opened this issue Mar 10, 2020 · 4 comments
Closed

Multiple Args include all arguments #570

backbone87 opened this issue Mar 10, 2020 · 4 comments
Labels
Community 👨‍👧 Something initiated by a community Duplicate 🔑 This issue or pull request already exists Enhancement 🆕 New feature or request

Comments

@backbone87
Copy link

Describe the bug
When using multiple Args parameter decorators, each of these parameters contain all args and not just the ones from their respective ArgsType.

To Reproduce

@ArgsType()
class SliceArgs {
  @Field(() => Int, { defaultValue: 0 })
  public skip!: number;

  @Field(() => Int, { defaultValue: 30 })
  public take!: number;
}

@ArgsType()
class MyObjectFilterArgs {
  @Field(() => Boolean, { defaultValue: null, nullable: true })
  public active!: boolean | null;
}

@Resolver()
class MyObjectQueries {
  public async myObjects(
    @Args()
    sliceArgs: SliceArgs,
    @Args()
    filterArgs: MyObjectFilterArgs,
  ): Promise<MyObject[]> {
    // sliceArgs and filterArgs both contain all 3 args: skip, take, active
    // isEqual(sliceArgs, { skip: 0, take: 30, active: null });
    // isEqual(filterArgs, { skip: 0, take: 30, active: null });
    
    // insteadof
    // isEqual(sliceArgs, { skip: 0, take: 30 });
    // isEqual(filterArgs, { active: null });

    return db.findMyObjects({ ...sliceArgs, ...filterArgs });
  }
}

Expected behavior
An args object should only contain the args from its respective ArgsType.

Enviorment (please complete the following information):

  • Image: node:12-alpine
  • Package version: 0.17.6
  • TypeScript version: 3.8.3

Additional context
While not documented, using multiple Args works as expected except for this issue

@MichalLytek
Copy link
Owner

While not documented, using multiple Args works as expected except for this issue

And because of that issue it's not supported thus prohibited to use more than one @Args at the same time.

@MichalLytek MichalLytek added Community 👨‍👧 Something initiated by a community Wontfix ❌ This will not be worked on labels Mar 10, 2020
@backbone87
Copy link
Author

backbone87 commented Mar 10, 2020

And because of that issue it's not supported thus prohibited to use more than one @Args at the same time.

i mean it works already? they even get the correct prototypes. the only issue are extrenous props on the arg object. should be quite straight forward to filter the props? and the presented use case is quite common imho.

please dont be so fast with rejecting stuff. I appreciate your work on this library and from what i see in the next branch i definitely like the direction you are taking this tool, but closing issues so quickly while not addressing the presented use case leaves a bitter taste one quite a bit frustrated. we can even rebrand this as a feature request, if that makes it better somehow.

edit: i can provide a PR for this. (you can also point me to the place where you want it implemented)

@MichalLytek
Copy link
Owner

MichalLytek commented Mar 10, 2020

Sorry but I have literally no time last days, so I can't deliberate more on that.

This issue is covered by the one with mapping names:
#263

If we have the mapping mechanism, it will automatically strip off all the additional properties as a side-effect, so no more changes needed to support that.
So for now you can't use that pattern, you need to wait a bit for this feature to be implemented.

I've added that to my vNext board to not forget about that 😉:
https://github.com/MichalLytek/type-graphql/projects/2#card-34384644

@MichalLytek MichalLytek added Duplicate 🔑 This issue or pull request already exists Enhancement 🆕 New feature or request and removed Wontfix ❌ This will not be worked on labels Mar 10, 2020
@backbone87
Copy link
Author

backbone87 commented Mar 11, 2020

np. I can work around the current behavior until next version is available. Thanks for reconsidering

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 Duplicate 🔑 This issue or pull request already exists Enhancement 🆕 New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants