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

fix: don't deduplicate argument types by argument name #162

Merged
merged 1 commit into from
May 23, 2024

Conversation

rbino
Copy link
Contributor

@rbino rbino commented May 23, 2024

As mentioned in the upgrade guide, the point of dropping auto generation of types was to avoid conflicts when two arguments had the same name but different types. Now that we require explicit generation, we can actually support that usecase for, e.g., arguments with the same name but different types in create/update mutations.

Contributor checklist

  • Bug fixes include regression tests
  • Features include unit/acceptance tests

As mentioned in the upgrade guide, the point of dropping auto generation of
types was avoiding conflicts when two arguments had the same name but different
types. Now that we require explicit generation, we can actually support that
usecase for, e.g., arguments with the same name but different types in
create/update mutations.
@rbino rbino marked this pull request as draft May 23, 2024 12:15
@rbino
Copy link
Contributor Author

rbino commented May 23, 2024

Drafted to check if we have to deduplicate for type instead, now that we removed deduplication for name

@zachdaniel
Copy link
Contributor

Looks great! Merge when ready 😁

@zachdaniel
Copy link
Contributor

Oh, interesting...I think we deduplicate in type name somewhere already? There are definitely already cases where people reuse the same enum/new type and it doesn't cause issues.

@rbino
Copy link
Contributor Author

rbino commented May 23, 2024

I'm hitting this: if I define a NewType with both graphql_type and graphql_input_type (with different types) and I use both (the first as a calculation output type, the second as argument input), Absinthe complains that the type is duplicate.

I'm checking to see if I can reproduce this in a test case.

@rbino rbino marked this pull request as ready for review May 23, 2024 12:56
@rbino
Copy link
Contributor Author

rbino commented May 23, 2024

So the duplication thing is actually orthogonal to this (I'm opening an issue about it), so this can be merged

@zachdaniel zachdaniel merged commit 2bc7b78 into ash-project:main May 23, 2024
15 checks passed
@zachdaniel
Copy link
Contributor

🚀 Thank you for your contribution! 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants