gateway: Support custom fetcher for RemoteGraphQLDataSource.
#4149
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@jhampton: 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/ |
…ementation, same as the gateway.
bd1a54a to
917c2c8
Compare
The relation is true right now, but that might change over time and this test name might end up out of date.
Currently, our test suite isn't as strongly typed as would be ideal. That means that even though this is a typing error, it's only at the editor level and won't affect compilation of the tests or their ability to pass. The actual typing here seems to be skewed because of problems with our `FetchMock` type that lives elsewhere in this repo. That error is causing a ripple affect of other typing errors in this `RemoteGraphQLDataSource` test, where there are dozens if not hundreds of similar errors. We will fix that typing outside of this PR (eventually; I hope soon), but for now I'm afraid that adding this annotation may inadvertently mask another error when we resolve the `FetchMock` type.
Due to the heavy-handed `Object.assign`-ing of all properties in the constructor, no property on this `RemoteGraphQLDataSource` is actually private since they can be provided and assigned directly over internally to the class itself. This is the reality of this pattern, which was - from what I understand - intended to prevent subclassing, but overlooked the security model provided by access modifiers and didn't consider the value or need to access a function's `this` binding. We need to fix this for other reasons, but for now, the `private` indicator actually causes typings in other use cases and creates a false sense of... well, privacy. This also aligns the behavior with, e.g., the `apq` option, which lives alongside.
We're already in `RemoteGraphQLDataSource`, and also, comply with the loose
expectation that `it ("reads like a sentence including the 'it' prefix")`.
:shrug:
fetcher for RemoteGraphQLDataSource.
abernix
added a commit
to apollographql/federation
that referenced
this pull request
Sep 4, 2020
…ographql/apollo-server#4149) Co-authored-by: Jesse Rosenberger <git@jro.cc> Apollo-Orig-Commit-AS: apollographql/apollo-server@489139c
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
add: Ability for the RemoteGraphQLDataSource to accept a fetcher implementation, the same as the Gateway (this is part 1 of 2, I think). The second sane step would be to use the Gateway's
fetcherwhen instantiating a RGDS. That said, you can do that like so without that additional change:Note: I added a new suite and test for the fetcher for sanity, but I'm seeing snapshot failures unrelated to my work here. Are these test failures expected right now?