# Type definitions for simple NPM package that generates OAuth 1.0a authorization header for Twitter API using native https nodejs module

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.

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;

• This is a great code review, really grateful you took the time to provide this feedback. Oct 5 '20 at 11:44