Skip to content

apollo-gateway export types #3371

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

Merged
merged 6 commits into from
Oct 7, 2019
Merged

apollo-gateway export types #3371

merged 6 commits into from
Oct 7, 2019

Conversation

wareczek
Copy link
Contributor

@wareczek wareczek commented Oct 3, 2019

No description provided.

@apollo-cla
Copy link

@wareczek: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/

@wareczek
Copy link
Contributor Author

wareczek commented Oct 5, 2019

Hi @trevor-scheer, please make it happen :)

@trevor-scheer
Copy link
Contributor

Hey @wareczek, thank you for submitting a PR! I'm not opposed to exporting these types, but as these functions are still experimental, I'm erring on the side of not (for now).

My primary justification for this is that TS can infer these types automatically for typical use cases. If you (or any other 👍'ers) would like to demonstrate a case where these types are needed, I'll consider the addition!

@wareczek
Copy link
Contributor Author

wareczek commented Oct 7, 2019

So be consequent. Callback experimental_didUpdateComposition has exported type DidUpdateCompositionCallback :P

https://github.com/apollographql/apollo-server/blob/master/packages/apollo-gateway/src/index.ts#L119

@abernix
Copy link
Member

abernix commented Oct 7, 2019

One of the side-effects of a strongly-typed language is that the corresponding auto-completion results in users using features which they (often, in my experience) don't realize are experimental or subject to change.

It's entirely possible that the prefix of the method name itself will make that clear, but I'm curious if it would hurt/help to also prefix the types with a similar prefix (e.g. Experimental_). So I ponder: Is that an okay pattern to use here? (I think so.) Common pattern? (Not sure!) Bad pattern? (I don't think so.)

Thoughts, anyone?

(Not saying we'll need to exercise this, but luckily, since TypeScript uses structural typing, even if a type is renamed later on it will still work just fine by implementors of the previous type, so long as the shape remains the same.)

@abernix
Copy link
Member

abernix commented Oct 7, 2019

Hmm, test failures do not seem to be spurious, but do not seem to be related to this PR? 🤔

@trevor-scheer
Copy link
Contributor

@wareczek, let's go ahead and export these types with the Experimental_ prefix rename to the related types (including the types that are already exported, as you've pointed out).

So I'm not ambiguous, I'll list them:

  • DidResolveQueryPlanCallback
  • DidFailCompositionCallback
  • DidUpdateCompositionCallback
  • UpdateServiceDefinitions
  • CompositionInfo

If you don't mind adding those prefixes, I'd be happy to merge this! Also a changelog entry would be greatly appreciated 😄

@trevor-scheer
Copy link
Contributor

trevor-scheer commented Oct 7, 2019

Hmm, test failures do not seem to be spurious, but do not seem to be related to this PR? 🤔

@abernix a re-run did pass the failing Node6 tests for me, so I'll chalk it up to spurious unless you saw something I didn't.

@trevor-scheer
Copy link
Contributor

Thank you @wareczek! 🎉

@trevor-scheer trevor-scheer merged commit 4d91f0c into apollographql:master Oct 7, 2019
abernix pushed a commit to apollographql/federation that referenced this pull request Sep 4, 2020
* Export gateway types related to experimental observability and control hooks.
* Prefix type names with `Experimental_`

Apollo-Orig-Commit-AS: apollographql/apollo-server@4d91f0c
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants