2
\$\begingroup\$

I added type definitions to a simple npm package. I would love to get some feedback on ways to do better for the type definitions.

The package exports a single function which returns a function that returns a promise:

function request(httpsOptions, body) {
  return new Promise((resolve, reject) => {
    const req = https.request(httpsOptions, (res) => {
      let data = ''
      res.on('data', (_data) => {
        data += _data
      })
      res.on('end', () => {
        resolve(JSON.parse(data))
      })
    })
    req.on('error', (error) => {
      reject(error)
    })
    req.write(body)
    req.end()
  })
}

module.exports = (oAuthOptions) => ({
  subdomain = 'api',
  endpoint,
  requestMethod = 'GET',
  queryParams = {},
  bodyParams = {},
}) => {
  const baseUrl = buildBaseUrl(subdomain, endpoint)
  const body = buildBody(bodyParams)
  const url = buildUrl(baseUrl, queryParams)
  const httpsOptions = buildHttpsOptions(url, {
    requestMethod,
    body,
    baseUrl,
    queryParams,
    bodyParams,
    oAuthOptions,
  })
  return request(httpsOptions, body)
}

This is the type definition file that I created:

export type OAuthOptions = {
  api_key: string
  api_secret_key: string
  access_token: string
  access_token_secret: string
}

export type RequestOptions = {
  subdomain?: string
  endpoint: string
  requestMethod?: string
  queryParams?: Record<string, string>
  bodyParams?: Record<string, string>
}

export default function (
  oAuthOptions: OAuthOptions,
): <T>(requestOptions: RequestOptions) => Promise<T>

Any advise on how to do better with the type definitions is highly appreciated.

\$\endgroup\$
1
\$\begingroup\$

Narrow the requestMethod Its type should be narrower. There are only a small number of possible methods. Instead of string, you should permit only those which make sense. You probably want something like:

requestMethod?: 'GET' | 'POST' | 'PUT' | 'DELETE' // etc

This will both reduce typos (eg, prevent someone from accidentally using 'PUST') and make it clear that the method should not just be any string, but one of a few particular strings.

String params? You have

queryParams?: Record<string, string>
bodyParams?: Record<string, string>

But, do both parameters really have to be only string keys with string values? That doesn't sound right. For example, I would hope and expect to be able to do:

queryParams: { 1: 'foo' } // turns into `1=foo`

or

queryParams: { foo: 1 } // turns info `foo=1`

or maybe even

queryParams: { foo: false } // turns info `foo=false`

If the library can handle those sorts of inputs and serialize them properly, which I'd hope it could, the type definitions should reflect that.


The JS can be improved a bit too:

Callbacks You can pass the reject callback alone to .on('error':

req.on('error', (error) => {
  reject(error)
})

can be

req.on('error', reject)

JSON parsing and errors

res.on('end', () => {
  resolve(JSON.parse(data))
})

What if data happens not to be JSON-parseable? Then an error will be thrown, but the Promise will not reject - it'll remain pending forever. Better to enclose that in a try/catch, and call reject if parsing fails.

Narrower response type? You currently have:

export default function (
  oAuthOptions: OAuthOptions,
): <T>(requestOptions: RequestOptions) => Promise<T>

without any constraint on T. You might consider explicitly requiring T to be a plain value when deserialized - that is, you wouldn't want people to be able to pass in a function type, or an object type where the object contains non-primitives (other than other plain objects and plain arrays). Maybe use something like AsJson in jcalz's answer here.

type AsJson<T> = 
  T extends string | number | boolean | null ? T : 
  T extends Function ? never : 
  T extends object ? { [K in keyof T]: AsJson<T[K]> } : 
  never;
\$\endgroup\$
1
  • \$\begingroup\$ This is a great code review, really grateful you took the time to provide this feedback. \$\endgroup\$ – MauricioRobayo Oct 5 '20 at 11:44

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.