StarpTech / apollo-datasource-http

Optimized JSON HTTP Data Source for Apollo Server
MIT License
73 stars 32 forks source link

fix-missing-request-option-type #11

Closed jasplin closed 3 years ago

jasplin commented 3 years ago

Hi again,

I have another pull request but while creating it I found that the request options were not "available" to the request and performRequest functions via typescript parameter types.

This pull request just fixes the missing types from the two request functions.

Thanks, John

StarpTech commented 3 years ago

Hi @jasplin, I don't understand the intention of this change. Why is that needed?

jasplin commented 3 years ago

The signature of the various fetching functions - for example, get:

  public async get<TResult = unknown>(  path: string, requestOptions?: RequestOptions ): Promise<Response<TResult>>{
    return this.request<TResult>({
      headers: {},
      ...requestOptions,
      method: 'GET',
      path,
      origin: this.baseURL,
    })
  }

(Note the spread operator on the requestOptions) Is then given to the function that actually initiates the get:

 private async request<TResult = unknown>(request: Request): Promise<Response<TResult>> {

There is no interfaces defined in this project that can be extended to modify the behaviour (ie request options). because Request is defined like:


export type Request = UndiciRequestOptions &
  CacheTTLOptions & {
    context?: Dictionary<string>
    headers: Dictionary<string>
    query?: Dictionary<string | number>
  }

The pull request I am going to propose adds an option to the request options like this:

export type RequestOptions = {
  context?: Dictionary<string>
  query?: Dictionary<string | number>
  body?: string | Buffer | Uint8Array | Readable | null
  headers?: Dictionary<string>
  signal?: AbortSignal | EventEmitter | null,
  // Should the response body be parsed as json content. Defaults to true if not provided for backwards compatibiity
  parseJSON?: boolean | null
} & CacheTTLOptions

Even if my upcoming pull request isn't accepted, I think the explicit typing will be helpful to others?

StarpTech commented 3 years ago

Hi @jasplin thanks for the PR. Based on your proposal I refactored it. Please check latest master.

// Should the response body be parsed as json content. Defaults to true if not provided for backwards compatibiity parseJSON?: boolean | null

Currently, the library only supports JSON. This is intentional.

Optimized JSON HTTP Data Source for Apollo Server

jasplin commented 3 years ago

OK great work, thanks. Happily closing this pull request