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

Updating the Data Sources docs #6847

Merged
merged 21 commits into from Aug 26, 2022
Merged

Updating the Data Sources docs #6847

merged 21 commits into from Aug 26, 2022

Conversation

rkoron007
Copy link
Contributor

Splitting out "Fetching from REST" from a more general article covering data sources.

@rkoron007 rkoron007 added the 📝 documentation Focuses on changes to the documentation (docs) label Aug 24, 2022
@rkoron007 rkoron007 self-assigned this Aug 24, 2022
@netlify
Copy link

netlify bot commented Aug 24, 2022

Deploy Preview for apollo-server-docs ready!

Name Link
🔨 Latest commit db9b9e4
🔍 Latest deploy log https://app.netlify.com/sites/apollo-server-docs/deploys/6308f51306d76100091de0f6
😎 Deploy Preview https://deploy-preview-6847--apollo-server-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@codesandbox-ci
Copy link

codesandbox-ci bot commented Aug 24, 2022

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit db9b9e4:

Sandbox Source
Apollo Server Typescript Configuration
Apollo Server Configuration

docs/source/data/fetching-rest.mdx Outdated Show resolved Hide resolved
docs/source/data/fetching-rest.mdx Outdated Show resolved Hide resolved
docs/source/data/fetching-rest.mdx Outdated Show resolved Hide resolved
docs/source/data/fetching-rest.mdx Outdated Show resolved Hide resolved
docs/source/data/fetching-rest.mdx Show resolved Hide resolved
docs/source/migration.mdx Outdated Show resolved Hide resolved
docs/source/migration.mdx Outdated Show resolved Hide resolved
docs/source/migration.mdx Outdated Show resolved Hide resolved
docs/source/migration.mdx Outdated Show resolved Hide resolved
docs/source/migration.mdx Outdated Show resolved Hide resolved
@rkoron007 rkoron007 marked this pull request as ready for review August 25, 2022 17:50
@rkoron007 rkoron007 requested a review from glasser August 25, 2022 21:17
Copy link
Member

@glasser glasser left a comment

Choose a reason for hiding this comment

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

looking pretty good. much of the challenge is that the existing doc page doesn't actually bother to explain the entire point of restdatasource (the fact that it's a two-level cache, one of which caches everything but only per request, and the other one respects HTTP response headers) at all... but sorry to not make sure you knew about Shane's stuff.

docs/source/data/fetching-data.mdx Outdated Show resolved Hide resolved
docs/source/migration.mdx Show resolved Hide resolved
willSendRequest(request: RequestOptions) {
request.headers.set('Authorization', this.token);
override willSendRequest(request: WillSendRequestOptions) {
request.headers['authorization'] = this.token;
Copy link
Member

Choose a reason for hiding this comment

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

we should probably mention somewhere that request now uses @apollo/fetcher like in #apolloutilsfetcher-replaces-apollo-server-env which means no Request/Headers methods

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a note above this code snippet:

New in Apollo Server 4: Apollo Server 4 now uses @apollo/fetcher under-the-hood, so you can no longer directly use Request or Headers methods.

Just to double-check, does this capture the point you are making above?

Copy link
Member

Choose a reason for hiding this comment

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

I guess, though it's more like we've actually changed the API folks write their this.post etc with. Compare the TS declarations:
https://www.runpkg.com/?apollo-datasource-rest@3.7.0/dist/RESTDataSource.d.ts
https://www.runpkg.com/?@apollo/datasource-rest@4.2.0/dist/RESTDataSource.d.ts

Like, the old API each of these take three args (path, body, other options) whereas in the new API it's two args (path, request where the body comes as a body option).

Additionally for hooks like willSendRequest, the object you have access to doesn't have Request/Header methods. But the bigger bit I guess is that the API changed! I think you have to adjust for body in the POST and PUT examples like you already did for patch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. I wasn't aware the args had for the HTTP convenience methods; thank you! So I'll remove this paragraph because it no longer applies:

For all methods, the third parameter is an init object that enables you to provide additional options (such as headers and referrers) to the fetch API that's used to send the request. For details, see MDN's fetch docs.

  1. I'm going to add this note to the "Intercepting Fetches" section to let folks know they can't access Request/Header methods directly:

New in Apollo Server 4: Apollo Server 4 now uses @apollo/fetcher under-the-hood for fetching, so hooks like willSendRequest no longer have direct access to Request or Headers methods.

  1. Great catch on that POST example, thanks!

Copy link
Member

Choose a reason for hiding this comment

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

For posterity, the package name is @apollo/utils.fetcher. (maybe you figured this out further down, I see this PR landed and I don't see any references to @apollo/fetcher 😄)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That did get updated, thanks for calling this out, though!

docs/source/migration.mdx Outdated Show resolved Hide resolved
docs/source/data/fetching-rest.mdx Outdated Show resolved Hide resolved
docs/source/data/fetching-rest.mdx Outdated Show resolved Hide resolved
docs/source/data/fetching-rest.mdx Show resolved Hide resolved
docs/source/data/fetching-rest.mdx Show resolved Hide resolved
docs/source/data/fetching-rest.mdx Outdated Show resolved Hide resolved
docs/source/migration.mdx Outdated Show resolved Hide resolved
docs/source/data/fetching-rest.mdx Outdated Show resolved Hide resolved

The **second parameter** depends on the HTTP method:

- For HTTP methods with a request body (`post`, `put`, `patch`), the second parameter is an _object_ that contains the `method` `cacheOptions`, and request `body` (e.g., `{ body: { id: movie.id } }`).
Copy link
Member

Choose a reason for hiding this comment

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

it actually can contain a lot of stuff like headers too? also missing comma after method

Copy link
Contributor Author

@rkoron007 rkoron007 Aug 26, 2022

Choose a reason for hiding this comment

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

Okay, you were so right—my bad for not rechecking the args! I replaced this whole section with:

Method parameters

For all HTTP convenience methods, the first parameter is the relative path of the endpoint you're sending the request to (e.g., movies). The second parameter is an object where you can set a request's headers, params, cacheOptions, and body:

class MoviesAPI extends RESTDataSource {
  override baseURL = 'https://movies-api.example.com/';
  
  // an example making an HTTP POST request
  async postMovie(movie) {
    return this.post(
      `movies`, // path
      { body: movie }, // request body
    );
  }
}

The **second parameter** depends on the HTTP method:

- For HTTP methods with a request body (`post`, `put`, `patch`), the second parameter is an _object_ that contains the `method` `cacheOptions`, and request `body` (e.g., `{ body: { id: movie.id } }`).
- For HTTP methods _without_ a request body, the second parameter is an object with keys and values corresponding to the request's query parameters.
Copy link
Member

Choose a reason for hiding this comment

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

Is that actually true? I think it's a similar object which can have headers and such, and params go on params?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(this is also addressed above)

docs/source/data/fetching-rest.mdx Outdated Show resolved Hide resolved
Co-authored-by: David Glasser <glasser@apollographql.com>
@rkoron007 rkoron007 merged commit 3a7ba05 into version-4 Aug 26, 2022
@rkoron007 rkoron007 deleted the RK/update-datasources-as4 branch August 26, 2022 16:42
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
📝 documentation Focuses on changes to the documentation (docs)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants