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 __typename to base TS and flow types #1701

Merged
merged 2 commits into from
May 15, 2019

Conversation

jefflewis
Copy link
Contributor

@jefflewis jefflewis commented Apr 11, 2019

As seen in these issues, __typename being included in the base type will help with comparing types returned by unions.

I hooked into the skipTypenames configuration flag. If undefined or false, __typename will be generated as an optional type in base types.

@jefflewis jefflewis changed the title Typenames Add __typename to base TS and flow types Apr 11, 2019
@dotansimha
Copy link
Owner

Hi @jefflewis , thank you for your contribution!
I'll review it very soon.

@kamilkisiela kamilkisiela self-requested a review April 16, 2019 13:53
Copy link
Collaborator

@kamilkisiela kamilkisiela left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should make skipTypename: true by default?

@kamilkisiela
Copy link
Collaborator

And maybe __typename should be only defined in interfaces?

@dotansimha
Copy link
Owner

@kamilkisiela why only for interfaces? I think we can add it for types as well.
My only concern is that now we should use Pick with __typename instead of manually adding it. But maybe we can do it after merging this PR.

@jefflewis
Copy link
Contributor Author

@kamilkisiela Adding __typename to the generated type is just as important here. Many of my generated GQL types are included in unions. Not having typename for these is exactly what I'm trying to fix.

@dotansimha I was unsure of a good way to refactor this to use a Pick, or to make it non-optional (note how the read-only flow type is not being used here) as I was also unsure of how to get that addressed.

It seems that both could be addressed in a future commit. Anyone needing typenames would be helped by this. and adding skipTypename: false would get the current behavior. It seems changing the default would be okay, but would similar "break" people's configs since the default is already true for operation types.

@kamilkisiela
Copy link
Collaborator

@jefflewis ok, seems reasonable.

About the default, I think since graphql-js doesn't add __typename property, that's what the developer does, then we shouldn't show it to people, it might be misleading.

{
  users: (parent, args, context, info) {
    // parent.__typename is accessible but undefined
  }
}

@jefflewis
Copy link
Contributor Author

jefflewis commented Apr 17, 2019

@kamilkisiela I'm not sure I understand what you mean here...

If __typename is output in the generated type, it will be accessible and defined. If the config is set to not generate, it will not be accessible or defined.

Changing the default to not generate __typename seems fine, but there's a lot more code to refactor to accomodate that change, since skipTypename is used in the typescript-operations plugin. Either way is fine with me, since all I need is to get the typename into the generated types! 😄

@jefflewis
Copy link
Contributor Author

I had to rebase after all of the packages moved into folders...

@cris-santos
Copy link

skipTypename is already on the docs (https://graphql-code-generator.com/docs/plugins/flow-operations) but the flow definitions aren't being generated with __typename. Am I missing something or it's not working because this PR is still open?

@tokenvolt
Copy link

@cristianosantos95 this is for GraphQL operations, i.e. query, mutation, subscription and fragment, but not for the GraphQL types.

@jefflewis
Copy link
Contributor Author

Is there anything I need to change about this before it gets merged and released? I'm more than willing to update the PR with any necessary changes.

@cris-santos
Copy link

@tokenvolt you mean this pull request or the one that's on the docs? Either way, none of my generated definitions have __typename (GraphQL operations and types). Is there a way to add it?

@tokenvolt
Copy link

@cristianosantos95 on the docs. This PR adds __typename to GraphQL objects as well.

@vasilich6107
Copy link

Hi! Thanks for library and PR.
I’m awaiting of this PR to be merged)

@dotansimha
Copy link
Owner

Hi @jefflewis , Sorry for the delay, I hope I'll find the time to merge it in the next days.

@jefflewis
Copy link
Contributor Author

No worries @dotansimha! Ping me if you need me to make any changes.

@Jonatthu
Copy link

Jonatthu commented May 9, 2019

@jefflewis Is this helping or related to this issue?
#643 (comment)

@andreyvital
Copy link

andreyvital commented May 10, 2019

@dotansimha sorry to push on this, but do you have an expectation of when to merge this PR? I'm not getting __typename in the generated definitions for TS. Thanks!

@beeequeue
Copy link

We are also waiting eagerly for this PR to be merged and released, we need it to migrate from Apollo's type generation

@dotansimha
Copy link
Owner

Hi all, sorry for the delay. I'm rebasing this PR manually now, and then I'll merge it to master. It still requires some changes because we need now to Pick<> the __typename as well, so it might take a bit more time (a day or so maybe) to have a stable release with it.

@dotansimha
Copy link
Owner

dotansimha commented May 15, 2019

I think, at the moment, I'll merge it as-is to allow the base plugins to add __typename, and x-operations plugins will add __typename by themself (like we do today).
The reason for that is might be a bit more complicated to do Pick<Type, '__typename'> when __typename? is optional, and then to remove the optional sign from it (in case the query contains __typename explicitly).
Everything should work as-is, and maybe in a future release we'll use Pick to get the __typename as well (it's just an optimization, so it's not required at the moment).

@jefflewis I did some minor changes to your PR (moved the __typename logic from DeclarationBlock to the BaseVisitor because I wanted it to be generic as possible).

Making sure now that everything works, and then I'll merge it :)

@@ -287,6 +291,7 @@ describe('TypeScript', () => {

expect(result).toBeSimilarStringTo(`
export type MyType = {
__typename?: "MyType",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be readonly __typename

Copy link
Owner

@dotansimha dotansimha Aug 27, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, thanks.

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.

10 participants