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

Feat | clarifying DefinitionsGeneratorOptions#defaultTypeMapping supported types #3210

Open
1 task done
Neosoulink opened this issue May 23, 2024 · 2 comments · May be fixed by #3346
Open
1 task done

Feat | clarifying DefinitionsGeneratorOptions#defaultTypeMapping supported types #3210

Neosoulink opened this issue May 23, 2024 · 2 comments · May be fixed by #3346
Labels

Comments

@Neosoulink
Copy link

Is there an existing issue that is already proposing this?

  • I have searched the existing issues

Is your feature request related to a problem? Please describe it

Using the schema first approach, I was caught on a small issue related to DefinitionsGeneratorOptions#defaultTypeMapping.
Yes, I passed the wrong type nomber instead of number to one of my scalar fields.

But I wonder if it will be better to just have clear type definitions for those scalar fields...

Describe the solution you'd like

inside the DefinitionsGeneratorOptions type, graphql-ast.explorer.ts#L66-L72 instead of defining the defaultTypeMapping property as follows:

Partial<
    Record<'ID' | 'Boolean' | 'Float' | 'String' | 'Int', string>
  >;

We can define a better definition like:

Partial<
    Record<'ID' | 'Float' | 'String' | 'Int', 'string' | 'number'> & Record<'Boolean', 'boolean' | 'number'>
 >;

Or use a concrete object type (something like { ID: 'number' | 'string' }).

💡 Of course, we can go much further by checking the passed type if it's not a supported typescript type...

Teachability, documentation, adoption, migration strategy

No response

What is the motivation / use case for changing the behavior?

As I mentioned in the above section, I made a mistake when I was defining the defaultTypeMapping.

I noticed the error very quickly and took me half of a second to solve it. However, I could have avoided it just with my typescript highlighter or with an error message.

This is just a very small improvement and maybe there's a reason behind this definition type choice, but I'll be grateful to receive your thoughts about this one.

@kamilmysliwiec
Copy link
Member

Would you like to create a PR for this?

@Neosoulink
Copy link
Author

Would you like to create a PR for this?

Yes, sure

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