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

Our docs' function signatures can get unreadable in a hurry. #21

Open
dead-claudia opened this issue Feb 9, 2019 · 7 comments
Open

Comments

@dead-claudia
Copy link
Member

dead-claudia commented Feb 9, 2019

Currently, we use a relatively unusual signature mechanism to document how our functions are called. It's nice and concise for a few simple cases, but it has a habit of getting in the way of readability and accuracy in the more complicated scenarios:

I've only covered about a third of the signatures in links above, and only individual bits of them in the bulleted list. But if you actually look at how it renders, I'd estimate about half of them have issues generated from the signature format itself.


What I'm really proposing here is that we should change our signature format to be much more readable. I'm thinking of something like this, using m() as an example:

### Signature

```js
vnode = m(selector, attrs = {}, ...children)
vnode = m(selector, attrs = {}, [...children])
```

- `selector` - A component or [simple CSS selector](https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_Selectors#Simple_selectors) string.
- `attrs` - A key/value map of HTML attributes, element properties, and/or [lifecycle methods](https://mithril.js.org/hyperscript.html#lifecycle-methods).
- `children` - A list of vnode children, including strings, numbers, vnodes, or arrays of vnode children. `null`, `undefined`, and `false` represent holes.

Returns a hyperscript vnode for Mithril to later [render](https://mithril.js.org/render.html).

[*How to read signatures*](https://mithril.js.org/signatures.md)

Rendered:

Signature

vnode = m(selector, attrs = {}, ...children)
vnode = m(selector, attrs = {}, [...children])
  • selector - A component or simple CSS selector string.
  • attrs - A key/value map of HTML attributes, element properties, and/or lifecycle methods.
  • children - A list of vnode children, including strings, numbers, vnodes, or arrays of vnode children. null, undefined, and false represent holes.

Returns a hyperscript vnode for Mithril to later render.

How to read signatures

For comparison, here's the existing docs:

`vnode = m(selector, attrs, children)`

Argument     | Type                                       | Required | Description
------------ | ------------------------------------------ | -------- | ---
`selector`   | `String|Object`                            | Yes      | A CSS selector or a [component](components.md)
`attrs`      | `Object`                                   | No       | HTML attributes or element properties
`children`   | `Array<Vnode>|String|Number|Boolean`       | No       | Child [vnodes](vnodes.md#structure). Can be written as [splat arguments](signatures.md#splats)
**returns**  | `Vnode`                                    |          | A [vnode](vnodes.md#structure)

[How to read signatures](signatures.md)

Rendered:

vnode = m(selector, attrs, children)

Argument Type Required Description
selector String|Object Yes A CSS selector or a component
attrs Object No HTML attributes or element properties
children Array<Vnode>|String|Number|Boolean No Child vnodes. Can be written as splat arguments
returns Vnode A vnode

How to read signatures

I'm open to other ideas, like using TypeScript types, as long as they're an improvement over what we currently have.

In case you're curious how I'd change m.request, here's how I'd do that:
### Signature

```js
promise = m.request(url, options = {})
promise = m.request({url, ...options})

promise.then(
    function(result) { ... },
    function(error) { ... }
)
```

- `url` - The URL to send the request to. It may be either absolute or relative, and it may contain [interpolations](#dynamic-urls).
- Options:
    - `options.method = "GET"` - The [HTTP method](https://developer.mozilla.org/en-US/docs/Web/HTTP/Methods) to use.
    - `options.data = undefined` - The data to be interpolated into the URL and serialized into the querystring (for GET requests) or body (for other types of requests).
    - `options.async = true` - Whether the request should be fired asynchronously.
    - `options.user = null` - The username for [HTTP basic authentication](https://developer.mozilla.org/en-US/docs/Web/HTTP/Authentication).
    - `options.password = null` - The password for [HTTP basic authentication](https://developer.mozilla.org/en-US/docs/Web/HTTP/Authentication).
    - `options.withCredentials = false` - Whether to send cookies to 3rd party domains.
    - `options.timeout = undefined` - The amount of milliseconds a request can take before automatically being [terminated](https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest/timeout).
    - `options.responseType = undefined` - The expected [type](https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest/responseType) of the response.
    - `options.config(xhr) -> newXhr = function(x) { return x }` - Exposes the underlying XMLHttpRequest object for low-level configuration.
    - `options.headers = {}` - Headers to append to the request before sending it, applied right before `options.config`.
    - `new options.type(response) -> result = ...` - A constructor to be applied to each object in the response if it's an array or the response itself otherwise. By default, the response is used directly.
    - `options.serialize(options.data) -> string = ...` - Serialize the data into a string. By default, this returns `options.data` if it's an instance of [`FormData`](https://developer.mozilla.org/en/docs/Web/API/FormData) or `JSON.stringify(options.data)` otherwise.
    - `options.deserialize(responseText) -> response = ...` - Deserialize `xhr.responseText` into the response data. By default, this returns `null` for empty responses and `JSON.parse(responseText)` otherwise.
    - `options.extract(xhr, options) -> response = ...` - A hook to specify how the [`XMLHttpRequest`](https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest) response should be read, useful for processing response data, reading headers, and cookies. By default, this returns `options.deserialize(xhr.responseText)` for requests that were successful (status either between 200 and 299 or 304), throws an error for requests that were not.
    - `options.useBody = (options.method !== "GET")` - Force the use of the HTTP body section for `data` in `GET` requests when set to `true`, or the use of querystring for other HTTP methods when set to `false`.
    - `options.background = false` - If `false`, redraws mounted components upon completion of the request. If `true`, it does not.

Returns a promise to the response, after it's been piped through `options.extract`, `options.deserialize`, and `options.type` as appropriate. Promise rejections outside the above hooks model request errors, and have a few special properties:

- `error.message` - The raw response text
- `error.code` - The server status code
- `error.response` - The parsed response

If a custom `options.extract` is provided, `deserialize` will be skipped and the value returned from the extract callback will be left as-is when the promise resolves. Furthermore, exceptions are *not* thrown when the server response status code indicates an error. Instead, it's expected that you determine in the callback whether the error was in fact a server error or a normal response.

[How to read signatures](https://mithril.js.org/signatures.md)

Rendered:

Signature

promise = m.request(url, options = {})
promise = m.request({url, ...options})

promise.then(
    function(result) { ... },
    function(error) { ... }
)
  • url - The URL to send the request to. It may be either absolute or relative, and it may contain interpolations.
  • Options:
    • options.method = "GET" - The HTTP method to use.
    • options.data = undefined - The data to be interpolated into the URL and serialized into the querystring (for GET requests) or body (for other types of requests).
    • options.async = true - Whether the request should be fired asynchronously.
    • options.user = null - The username for HTTP basic authentication.
    • options.password = null - The password for HTTP basic authentication.
    • options.withCredentials = false - Whether to send cookies to 3rd party domains.
    • options.timeout = undefined - The amount of milliseconds a request can take before automatically being terminated.
    • options.responseType = undefined - The expected type of the response.
    • options.config(xhr) -> newXhr = function(x) { return x } - Exposes the underlying XMLHttpRequest object for low-level configuration.
    • options.headers = {} - Headers to append to the request before sending it, applied right before options.config.
    • new options.type(response) -> result = ... - A constructor to be applied to each object in the response if it's an array or the response itself otherwise. By default, the response is used directly.
    • options.serialize(options.data) -> string = ... - Serialize the data into a string. By default, this returns options.data if it's an instance of FormData or JSON.stringify(options.data) otherwise.
    • options.deserialize(responseText) -> response = ... - Deserialize xhr.responseText into the response data. By default, this returns null for empty responses and JSON.parse(responseText) otherwise.
    • options.extract(xhr, options) -> response = ... - A hook to specify how the XMLHttpRequest response should be read, useful for processing response data, reading headers, and cookies. By default, this returns options.deserialize(xhr.responseText) for requests that were successful (status either between 200 and 299 or 304), throws an error for requests that were not.
    • options.useBody = (options.method !== "GET") - Force the use of the HTTP body section for data in GET requests when set to true, or the use of querystring for other HTTP methods when set to false.
    • options.background = false - If false, redraws mounted components upon completion of the request. If true, it does not.

Returns a promise to the response, after it's been piped through options.extract, options.deserialize, and options.type as appropriate. Promise rejections outside the above hooks model request errors, and have a few special properties:

  • error.message - The raw response text
  • error.code - The server status code
  • error.response - The parsed response

If a custom options.extract is provided, deserialize will be skipped and the value returned from the extract callback will be left as-is when the promise resolves. Furthermore, exceptions are not thrown when the server response status code indicates an error. Instead, it's expected that you determine in the callback whether the error was in fact a server error or a normal response.

How to read signatures

For comparison, here's the original docs:

### Signature

`promise = m.request([url,] options)`

Argument                  | Type                              | Required | Description
------------------------- | --------------------------------- | -------- | ---
`url`                     | `String`                          | No       | If present, it's equivalent to having the options `{method: "GET", url: url}`. Values passed to the `options` argument override options set via this shorthand.
`options.method`          | `String`                          | No       | The HTTP method to use. This value should be one of the following: `GET`, `POST`, `PUT`, `PATCH`, `DELETE`, `HEAD` or `OPTIONS`. Defaults to `GET`.
`options.url`             | `String`                          | Yes      | The URL to send the request to. The URL may be either absolute or relative, and it may contain [interpolations](#dynamic-urls).
`options.data`            | `any`                             | No       | The data to be interpolated into the URL and serialized into the querystring (for GET requests) or body (for other types of requests).
`options.async`           | `Boolean`                         | No       | Whether the request should be asynchronous. Defaults to `true`.
`options.user`            | `String`                          | No       | A username for HTTP authorization. Defaults to `undefined`.
`options.password`        | `String`                          | No       | A password for HTTP authorization. Defaults to `undefined`. This option is provided for `XMLHttpRequest` compatibility, but you should avoid using it because it sends the password in plain text over the network.
`options.withCredentials` | `Boolean`                         | No       | Whether to send cookies to 3rd party domains. Defaults to `false`
`options.timeout`         | `Number`                          | No       | The amount of milliseconds a request can take before automatically being [terminated](https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest/timeout).  Defaults to `undefined`.
`options.responseType`    | `String`                          | No       | The expected [type](https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest/responseType) of the response. Defaults to `undefined`.
`options.config`          | `xhr = Function(xhr)`             | No       | Exposes the underlying XMLHttpRequest object for low-level configuration. Defaults to the [identity function](https://en.wikipedia.org/wiki/Identity_function).
`options.headers`         | `Object`                          | No       | Headers to append to the request before sending it (applied right before `options.config`).
`options.type`            | `any = Function(any)`             | No       | A constructor to be applied to each object in the response. Defaults to the [identity function](https://en.wikipedia.org/wiki/Identity_function).
`options.serialize`       | `string = Function(any)`          | No       | A serialization method to be applied to `data`. Defaults to `JSON.stringify`, or if `options.data` is an instance of [`FormData`](https://developer.mozilla.org/en/docs/Web/API/FormData), defaults to the [identity function](https://en.wikipedia.org/wiki/Identity_function) (i.e. `function(value) {return value}`).
`options.deserialize`     | `any = Function(string)`          | No       | A deserialization method to be applied to the `xhr.responseText`. Defaults to a small wrapper around `JSON.parse` that returns `null` for empty responses. If `extract` is defined, `deserialize` will be skipped.
`options.extract`         | `any = Function(xhr, options)`    | No       | A hook to specify how the XMLHttpRequest response should be read. Useful for processing response data, reading headers and cookies. By default this is a function that returns `xhr.responseText`, which is in turn passed to `deserialize`. If a custom `extract` callback is provided, the `xhr` parameter is the XMLHttpRequest instance used for the request, and `options` is the object that was passed to the `m.request` call. Additionally, `deserialize` will be skipped and the value returned from the extract callback will be left as-is when the promise resolves. Furthermore, when an extract callback is provided, exceptions are *not* thrown when the server response status code indicates an error.
`options.useBody`         | `Boolean`                         | No       | Force the use of the HTTP body section for `data` in `GET` requests when set to `true`, or the use of querystring for other HTTP methods when set to `false`. Defaults to `false` for `GET` requests and `true` for other methods.
`options.background`      | `Boolean`                         | No       | If `false`, redraws mounted components upon completion of the request. If `true`, it does not. Defaults to `false`.
**returns**               | `Promise`                         |          | A promise that resolves to the response data, after it has been piped through the `extract`, `deserialize` and `type` methods

[How to read signatures](signatures.md)

Rendered:

Signature

promise = m.request([url,] options)

Argument Type Required Description
url String No If present, it's equivalent to having the options {method: "GET", url: url}. Values passed to the options argument override options set via this shorthand.
options.method String No The HTTP method to use. This value should be one of the following: GET, POST, PUT, PATCH, DELETE, HEAD or OPTIONS. Defaults to GET.
options.url String Yes The URL to send the request to. The URL may be either absolute or relative, and it may contain interpolations.
options.data any No The data to be interpolated into the URL and serialized into the querystring (for GET requests) or body (for other types of requests).
options.async Boolean No Whether the request should be asynchronous. Defaults to true.
options.user String No A username for HTTP authorization. Defaults to undefined.
options.password String No A password for HTTP authorization. Defaults to undefined. This option is provided for XMLHttpRequest compatibility, but you should avoid using it because it sends the password in plain text over the network.
options.withCredentials Boolean No Whether to send cookies to 3rd party domains. Defaults to false
options.timeout Number No The amount of milliseconds a request can take before automatically being terminated. Defaults to undefined.
options.responseType String No The expected type of the response. Defaults to undefined.
options.config xhr = Function(xhr) No Exposes the underlying XMLHttpRequest object for low-level configuration. Defaults to the identity function.
options.headers Object No Headers to append to the request before sending it (applied right before options.config).
options.type any = Function(any) No A constructor to be applied to each object in the response. Defaults to the identity function.
options.serialize string = Function(any) No A serialization method to be applied to data. Defaults to JSON.stringify, or if options.data is an instance of FormData, defaults to the identity function (i.e. function(value) {return value}).
options.deserialize any = Function(string) No A deserialization method to be applied to the xhr.responseText. Defaults to a small wrapper around JSON.parse that returns null for empty responses. If extract is defined, deserialize will be skipped.
options.extract any = Function(xhr, options) No A hook to specify how the XMLHttpRequest response should be read. Useful for processing response data, reading headers and cookies. By default this is a function that returns xhr.responseText, which is in turn passed to deserialize. If a custom extract callback is provided, the xhr parameter is the XMLHttpRequest instance used for the request, and options is the object that was passed to the m.request call. > Additionally, deserialize will be skipped and the value returned from the extract callback will be left as-is when the promise resolves. Furthermore, when an extract callback is provided, exceptions are not thrown when the server response status code indicates an error.
options.useBody Boolean No Force the use of the HTTP body section for data in GET requests when set to true, or the use of querystring for other HTTP methods when set to false. Defaults to false for GET requests and true for other methods.
options.background Boolean No If false, redraws mounted components upon completion of the request. If true, it does not. Defaults to false.
returns Promise A promise that resolves to the response data, after it has been piped through the extract, deserialize and type methods

How to read signatures

@dead-claudia
Copy link
Member Author

Basically, it's moving crap from tables to code blocks with associated prose so it's easier to follow. My proposal could be summarized as this:

  • Allow defaults to be specified in the declaration.
  • Move the descriptions out of the table, so they don't interfere with the "gist" that's the code block.
  • Make the "optionality" of a parameter coupled to the parameter and not the description.
  • Document the return value separately from the parameters.
  • Leave types implied when they're clear from context. Precisely typing stuff is what @types/mithril is for, and most of the time when the type needs explicitly stated, the type of the value/result itself has additional semantics we need to document.
  • Move complicated semantic explanations out of the overview and parameter descriptions.

@spacejack
Copy link
Contributor

Agreed. I've edited the request docs once or twice and it's not fun in its current form. :)

@delventhalz
Copy link

delventhalz commented Jul 11, 2019

Hey there. Long time listener, first time caller. I'm thinking about picking this up just to get my feet wet. How would you feel about modifying the original suggestion slightly to more closely monkey how MDN formats their docs? I'm a big fan of stealing from the best. Something like this:

### Syntax

```
m(selector[, attrs[, ...children]])
```

#### Parameters

- `selector`

  A component or [simple CSS selector](https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_Selectors#Simple_selectors) string.

- `attrs` | _optional_

  A key/value map of HTML attributes, element properties, and/or [lifecycle methods](https://mithril.js.org/hyperscript.html#lifecycle-methods).

- `children` | _optional_

  A list of vnode children, including strings, numbers, vnodes, or arrays of vnode children. `null`, `undefined`, and `false` represent holes.

#### Return value

A hyperscript vnode for Mithril to later [render](https://mithril.js.org/render.html).

[*How to read signatures*](https://mithril.js.org/signatures.md)

And rendered:

Syntax

m(selector[, attrs[, ...children]])

Parameters

  • selector

    A component or simple CSS selector string.

  • attrs | optional

    A key/value map of HTML attributes, element properties, and/or lifecycle methods.

  • children | optional

    A list of vnode children, including strings, numbers, vnodes, or arrays of vnode children. null, undefined, and false represent holes.

Return value

A hyperscript vnode for Mithril to later render.

How to read signatures

I think this could offer a few advantages:

  • Use a commonly understood function signature notation
  • Clearly separate parameters from return value
  • Clearly separate parameter names and description, allowing for longer descriptions

@orbitbot
Copy link
Member

orbitbot commented Jul 11, 2019

IMO the original proposal works for most stuff, but m.request is just messy no matter what if everything is explained at once. Might be easier to read with the MDN syntax perhaps. Otherwise, I guess people might be somewhat familiar with typescript annotations by now, but I don't really have enough experience with those to say if that is some kind of format that might be useful for this level of documentation (?).

The MDN style for XHR is of a similar length to m.request and at least a bit better than the table and wall of bullet points: https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest

@dead-claudia
Copy link
Member Author

@orbitbot We could hide the bullet points by just wrapping the signature in a <div class="signature"> and a single line of CSS.

As for TS-like vs MDN, I'm neutral. I personally prefer to avoid explicit type names, sticking a little closer to MDN with a few TS/ES things like optional? and ...rest arguments, but I'm not beholden to that. I've found variable names to be sufficient to convey types, providing the flexibility you'd get out of TS without necessarily using a TS type. I would, however, be against going as far as to use TS generics, interface declarations, and the like.

And MDN's use of method(arg[, optional]) in its JS docs is based on the ES spec which uses a similar idiom. The DOM uses WebIDL, obviously, and MDN generally prefers that for its HTML/DOM API documentation. I don't see the ES spec style very often elsewhere (I normally see optional?, optional = value, or prose - jQuery is an exception), and I only usually see the WebIDL style in older libraries and utilities like Uglify and a few other things. BTW, our current documentation style is very jQuery-like, so we aren't exactly on an island. The main difference is they use rows and line breaks, where we use a table.

Something aligned with what jQuery does would look like this:

`m(selector [, attrs] [, child1] [, childN])`

*Returns: a [vnode](vnodes.md#structure)`*

- `selector`<br>
  Type: String or Object<br>
  A CSS selector or a [component](components.md)

- `attrs`<br>
  Type: Object<br>
  HTML attributes or element properties

- `child1`<br>
  Type: String, Number, Boolean, a vnode, or an array of children<br>
  A child [vnode](vnodes.md#structure).

- `childN`<br>
  Type: String, Number, Boolean, a vnode, or an array of children<br>
  Additional child [vnodes](vnodes.md#structure).

[How to read signatures](signatures.md)

Rendered:

m(selector [, attrs] [, child1] [, childN])

Returns: a vnode`

  • selector
    Type: String or Object
    A CSS selector or a component

  • attrs
    Type: Object
    HTML attributes or element properties

  • child1
    Type: String, Number, Boolean, a vnode, or an array of children
    A child vnode.

  • childN
    Type: String, Number, Boolean, a vnode, or an array of children
    Additional child vnodes.

How to read signatures

@orbitbot
Copy link
Member

To be clear, my issue with the documentation in the case of something like the amount of options that m.request has is that it isn't readable, not that there are bullet points there. Since the amount of text is so large, there needs to be some kind of separation between each bullet point for it to be even remotely legible, so in that case the MDN style is definitely better.

Without proper spacing for the "wall of bullet points", it's essentially like reading a long text without typography, where indentation and line breaks between paragraphs have been removed.

I don't really have that much issue with MDN or jQuery style, other than that with a lot of options it starts to consume so much vertical space that the reader might lose the context. Not sure how relevant the subheadings are in MDN's XHR docs, but I don't really keep track of what section I'm reading, and this is with zooming out (Chrome's site zooming) a bit so I still have more vertical space visible.

@delventhalz
Copy link

delventhalz commented Jul 12, 2019

If the parameter description is in its own paragraph, it gives you room to inline a type annotation if you wanted to include it:

  • selector: String|Object
    A CSS selector or a component

Though it might be clearer with a JSDoc style annotation:

  • selector {String|Object}
    A CSS selector or a component

A few ways to include an optional tag with an inline type too:

  • attrs?: Object
    A key/value map of HTML attributes, element properties, and/or lifecycle methods.
  • attrs {Object} (optional)
    A key/value map of HTML attributes, element properties, and/or lifecycle methods.
  • attrs - {Object} - optional
    A key/value map of HTML attributes, element properties, and/or lifecycle methods.

As for the function signature itself, I think the use of brackets to denote optional parameters is really common and worth replicating. Not really particular on whether those are nested (MDN), sequential (jQuery) or whatever.

I don't think trying to make the signature more JS-like (as in the original suggestion) is a great approach. It's not really valid JS as written anyway. So I would personally rather have some good example code which is 100% valid, best-practice, JS, and then something distinct but clear which documents the signature.

@dead-claudia dead-claudia transferred this issue from MithrilJS/mithril.js Sep 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Low priority
Development

No branches or pull requests

4 participants