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

Add graphql_type for ash_graphql #12

Closed
wants to merge 1 commit into from
Closed

Conversation

skanderm
Copy link

@skanderm skanderm commented Jun 28, 2024

Adds a missing graphql_type/1 function definition for https://hexdocs.pm/ash_graphql/AshGraphql.Type.html#c:graphql_type/1, as ash_graphql errors out here for AshUUID.UUID attributes: https://github.com/ash-project/ash_graphql/blob/main/lib/resource/resource.ex#L4302

Please feel free to close this if you'd prefer another solution!

@moissela
Copy link
Member

moissela commented Jul 1, 2024

Hi @skanderm, thanks for your contribution!

Not using GraphQL and AshGraphql at the moment, I don't know how to evaluate the impact of this PR or whether it is correct to add that method directly into the extension.

Maybe @zachdaniel can give us an hint on this?

@zachdaniel
Copy link
Contributor

The isn't how I'd suggest doing this. Instead, you can override types at the resource level.

@zachdaniel
Copy link
Contributor

Probably? I'd have to think about it 🤔 I'd suggest for now holding off on this front, even if we did do it it would be :string, not :id I think?

@skanderm
Copy link
Author

skanderm commented Jul 3, 2024

Happy to close this in favor of another solution

@zachdaniel when it comes to overriding types at the resource level, are you talking about attribute_types in ash_graphql? Because if so, I think there may be a bug there when applying attribute_types [id: :string]. I can dig into it and add a breaking test if so. Specifically, I get this error:

** (RuntimeError) Could not determine graphql type for AshUUID.UUID, please define: graphql_type/1!

    (ash_graphql 1.2.0) lib/resource/resource.ex:4186: AshGraphql.Resource.do_field_type/5
    (ash_graphql 1.2.0) lib/resource/resource.ex:3661: anonymous fn/5 in AshGraphql.Resource.attributes/4
    (elixir 1.16.0) lib/enum.ex:1700: Enum."-map/2-lists^map/1-1-"/2
    (elixir 1.16.0) lib/enum.ex:1700: Enum."-map/2-lists^map/1-1-"/2
    (ash_graphql 1.2.0) lib/resource/resource.ex:3658: AshGraphql.Resource.attributes/4
    (ash_graphql 1.2.0) lib/resource/resource.ex:3580: AshGraphql.Resource.fields/5
    (ash_graphql 1.2.0) lib/resource/resource.ex:3557: AshGraphql.Resource.type_definition/5
    (ash_graphql 1.2.0) lib/resource/resource.ex:1768: AshGraphql.Resource.type_definitions/5

@skanderm skanderm closed this Jul 3, 2024
@zachdaniel
Copy link
Contributor

🤔 yeah that is what I was referring to, perhaps there is an issue there in that case.

@skanderm
Copy link
Author

skanderm commented Jul 3, 2024

Probably? I'd have to think about it 🤔 I'd suggest for now holding off on this front, even if we did do it it would be :string, not :id I think?

Oh and whether it's a :string or :id, the docs say:

ID: The ID scalar type represents a unique identifier, often used to refetch an object or as the key for a cache. The ID type is serialized in the same way as a String; however, defining it as an ID signifies that it is not intended to be human‐readable.

I'm not quite sure whether different clients handle that differently for caching purposes though. (edit: pretty sure react-query just caches by request key + params, so it may not matter for others).

@zachdaniel
Copy link
Contributor

Yeah, I think it makes sense for id to be used as a type, just not sure that it makes sense for a type itself to express that it is an id.

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.

3 participants