Comment typings with documentation

[demurgos]

We should adopt the style of other typings and include the related documentation in the comments.
It would greatly help in providing context to the typings: it's convenient to jump straight to the definitions and read the doc. This would be a first step in the direction of integrating documentation.

It would also have prevented issues such as #3.

[louy]

I totally agree. I've copied this one from DT without changing anything really. I always copy everything from docs as I write typings. e.g. node-uuid I wrote recently.

[demurgos]

Right, it's the same for me.
Once the cookie issue is settled, I'll add comments to these definitions.

[louy]

@demurgos I've added some docs in #6.

[demurgos]

Nice, thank you.

[demurgos]

Okay, so I ran in some new issues again with the result of request(...) not being always an http.IncomingMessage (I received the result of this function).

Even if request is a big package, their documentation is a huge mess...
I'm seriously planning to start from your documented version and simply rewrite everything, and not only comment with the doc but also with the source code itself.

[louy]

Hmm. I don't see how you can possibly get that as it's only ever used here

  response.toJSON = responseToJSON

and here.

Request.prototype.toJSON = requestToJSON

Can you show me how to reproduce that?

I totally agree. request is documented in a very very horrible way. 😑

I think rewriting everything is a good idea as there are some deprecated options still in the declaration file (like request.forever). Though I don't like the idea of pasting the source code in as it means more effort maintaining it.

[demurgos]

I am trying to convert a lib to Typescript so I don't know myself how it ends up receiving the JSON value (I am currently investigating it), I let you know once I found. It's something like:

var options = {
    headers: getHeaders(url),
    timeout: 60000,
    qs: qs,
    url: url,
    method: "GET",
    jar: jar,
    gzip: true
};
request(options, (err, res, body) => { 
    // and here, res has all the attributes of the function I linked
});

You're right about the code, I may have spoken a bit too fast: it's hard to maintain. But maybe adding some of our own comments or some pseudo-js to clarify edge cases could be useful.

[demurgos]

I finally found some time for this issue, here is a test I made:

const URL = "https://api.typings.org/entries/npm/request";

let options: request.CoreOptions & request.UriOptions = {
  headers: {},
  timeout: 60000,
  qs: {},
  uri: URL,
  method: "GET",
  jar: null,
  gzip: true
};

request(options, (error, response, body) => {
  if (error) {
    return null;
  }
  console.log(Object.keys(response));
});

This does return:

[
  '_readableState',
  'readable',
  'domain',
  '_events',
  '_eventsCount',
  '_maxListeners',
  'socket',
  'connection',
  'httpVersionMajor',
  'httpVersionMinor',
  'httpVersion',
  'complete',
  'headers',
  'rawHeaders',
  'trailers',
  'rawTrailers',
  'upgrade',
  'url',
  'method',
  'statusCode',
  'statusMessage',
  'client',
  '_consuming',
  '_dumped',
  'req',
  'request',
  'toJSON',
  'caseless',
  'read',
  'body'
]

So in the end, I do not receive a JSONRequest, but this is not a pure IncomingMessage either. All the keys after request are added by request.
For example, what does this line do ? There is not a single mention of it in their README.md.

Here are the ones responsible for request and toJSON: clic
And here they are adding body.

[demurgos]

I don't really know what to do about these methods and properties. They are not documented anywhere but I found some repos using them as I told you.
The best would be to update these repo to only rely on the documented properties, but what about the definitions ?

[demurgos]

I asked more informations about these properties on the request repo.

I haven't done a PR yet, but I started to work on this branch. The main changes for the moment are about replicating a part of the structure of the source package as discussed here. I prefer to add some tests before submitting a PR. (The changes are non-breaking)

I was also confused at first about the way this module exposes its static API with a generic, but now I get it.

[louy]

So caseless looks like a copy of the whole object with case insensitive keys (like headers).

  caseless:
   Caseless {
     dict:
      { date: 'Tue, 03 May 2016 20:56:46 GMT',
        'content-type': 'application/json; charset=utf-8',
        'transfer-encoding': 'chunked',
        connection: 'close',
        'set-cookie': [Object],
        'x-powered-by': 'Express',
        'typings-client-id': '6ff5bba3-4dc2-4e21-9d25-f269a50cf59e',
        etag: 'W/"76-n3q30a2Z650Fu8Y6Tk4NYw"',
        via: '1.1 vegur',
        server: 'cloudflare-nginx',
        'cf-ray': '29d69f1c5b693596-LHR',
        'content-encoding': 'gzip' } },
  read: [Function],
  body: '{"name":"request","source":"npm","homepage":null,"description":null,"updated":"2016-04-28T22:37:25.000Z","versions":1}' }

it's apparently used internally like here.

Also response is an instance of IncomingMessage. It's just those extra props.

request(options, (error, response, body) => {
  if (error) {
    return null;
  }
  console.log(require('util').inspect(response));
});
//  > IncomingMessage {
//    ...
//  }

I agree. Let's only add documented features. Looking forward to seeing it 😉

[demurgos]

I have some good news, the team behind request (especially @simov) is pretty active and they are preparing a website with documentation for this package so they seem interested in an update. I am trying to help them to migrate the current API to their website so we should have a clearer documentation soon.

References:

• Original issue
• Website repo
• My branch (still work in progress)

[demurgos]

I'm closing this issue because it is no longer relevant. There are better issues on the Request repos and and typings is no longer very actively developed.