Node: A collection of minor issues with the docs

[davidgilbertson]

• Version: 7
• Platform: NA
• Subsystem: NA

Hi, I've just finished reading the docs and made a few notes about some minor issues I found. I thought I'd dump them here, I can do a PR if appropriate. I might be misunderstanding some things so didn't want to jump in.

1. Should tls.connect() list host and port in the options if they're already params?
   https://nodejs.org/api/tls.html#tls_tls_connect_port_host_options_callback

2. Seems weird that path.resolve() arguments are optional. Worth documenting that the method returns '.' if you pass nothing in?
   https://nodejs.org/api/path.html#path_path_join_paths
   https://nodejs.org/api/path.html#path_path_resolve_paths

3. The params for path.format() and path.parse() are in different orders, format() is logically wrong based on the diagram under parse()
   https://nodejs.org/api/path.html#path_path_format_pathobject
   https://nodejs.org/api/path.html#path_path_parse_path

4. There shouldn't be spaces in tlsSocket.getPeerCertificate([ detailed ])
   https://nodejs.org/api/tls.html#tls_tlssocket_getpeercertificate_detailed

5. net.Server shows connections as deprecated...
   https://nodejs.org/api/net.html#net_server_connections
   tls.Server inherits from this, but doesn't show connections as deprecated
   https://nodejs.org/api/tls.html#tls_server_connections
   Should it?

6. Is the encoding parameter allowed if the data parameter isn't set?
   Should: request.end([data][, encoding][, callback])
   be: request.end([data[, encoding]][, callback])
   ?
   https://nodejs.org/api/http.html#http_request_end_data_encoding_callback

7. dgram has a header "dgram module functions" before the static methods, none of the other pages do this. Is the dgram module different, or is this just an inconsistency?
   https://nodejs.org/api/dgram.html#dgram_dgram_module_functions

8. dgram.createSocket() has two different signatures listed. One with type, one with object.
   https://nodejs.org/api/dgram.html#dgram_dgram_createsocket_type_callback
   That's cool, but the same sort of thing in the fs module where an options parameter could be a string defining the encoding, or an object that has an encoding property and something else is shown as a single signature.
   https://nodejs.org/api/fs.html#fs_fs_readfile_file_options_callback
   IMO they should both be like dgram, that is, fs.readFile(file[, encoding], callback) should be listed in addition to fs.readFile(file[, options], callback).
   But that means lots to change on the fs page.

9. There is (very?) minor inconsistency in callback terminology. E.g. socket connect specifies a connectListener which is nice, it defines that this is a callback that is the same as 'listening' for the connect event.
   https://nodejs.org/api/net.html#net_socket_connect_options_connectlistener
   But server.listen names the parameter just callback.
   https://nodejs.org/api/net.html#net_server_listen_options_callback
   Personally my choice would be the 'on' prefix followed by the event name that would trigger it. onConnect, onListening etc. I think that would make understanding when the callbacks are called easy.

10. The example of a dgam sending message unnecessarily converts a string to a buffer, since it will be converted to a (utf8) Buffer anyway. Less complexity in examples is better, right?
    (Also it vaguely implies that a string isn't as valid as a buffer.)
    https://nodejs.org/api/dgram.html#dgram_socket_send_msg_offset_length_port_address_callback

11. Parameter names are almost always camelCase, except in crypto where they're snake_case.
    https://nodejs.org/api/crypto.html#crypto_cipher_final_output_encoding
    Is that an inconsistency or is there meaning in that?

12. Using util.inspect() inside console.log() is redundant (I think. Isn't it?)
    https://nodejs.org/api/events.html#events_emitter_listeners_eventname
    https://nodejs.org/api/vm.html#vm_vm_runincontext_code_contextifiedsandbox_options
    https://nodejs.org/api/vm.html#vm_script_runinnewcontext_sandbox_options

13. Missing apostrophe in "run using the vm modules methods"
    https://nodejs.org/api/vm.html#vm_what_does_it_mean_to_contextify_an_object

14. I have no idea about this one, but is sandbox really optional in script.runInNewContext()?
    If I call script.runInNewContext(options), wouldn't it try and make a sandbox out of the options object? I went looking through the source but found myself in a strange land and gave up.
    https://nodejs.org/api/vm.html#vm_script_runinnewcontext_sandbox_options

15. zlib.constants is missing "added in version..." (and is quite new so this is a problem)
    https://nodejs.org/api/zlib.html#zlib_zlib_constants
    Looks like this commit
    https://github.com/nodejs/node/commit/197a4652807bf1d830d67c1d8a4aecdf35fda6a4#diff-c245d87dba893de0b77c7574f0081633R388

16. zlib.codes isn't documented. Is that on purpose?
    https://nodejs.org/api/zlib.html

17. zlib methods all have options as an argument, but not shown in square brackets (but in the TOC they're shown in square brackets). Less important, but having the word in the argument list be linked is inconsistent with the other pages. The description contains the word 'options' which is linked to the class options section.

18. Wording is strange for zlib methods, e.g. "Returns a new Deflate object with an options."
    By comparison, in net.connect() the equivalent wording is something like "The options are passed to the net.Socket constructor"
    https://nodejs.org/api/zlib.html

19. Getting picky here ... but the wording for there being Sync version of methods is different for fs and zlib.
    https://nodejs.org/api/zlib.html#zlib_convenience_methods
    https://nodejs.org/api/fs.html#fs_file_system

[sam-github]

These all look valid and excellent feedback to me, though I didn't look at 3 closely, you didn't say what is wrong.

Its a lot of work, but I would request you PR each fix individually. Well, perhaps the zlib could be 5 commits in a single PR.

Commenting on a list like above is not easy with the github UI, commenting on 19 issues would've been much easier, but at this point, I think going directly to a PR, not an issue, would be faster for you, and faster to get review comments, and get these fixed.

11 is historical, not meaningful, should be fixed

10 is probably because the dgram API didn't used to accept strings

[sam-github]

And you can't be too picky about the docs, they should be as clear as possible.

[bnoordhuis]

I have no opinions on style issues but as to the technical side of things:

tls.Server inherits from this, but doesn't show connections as deprecated [..] Should it?

Yes.

Is the encoding parameter allowed if the data parameter isn't set? Should: request.end([data][, encoding][, callback]) be: request.end([data[, encoding]][, callback])

The last one is the correct one, yes.

The example of a dgam sending message unnecessarily converts a string to a buffer, since it will be converted to a (utf8) Buffer anyway.

I think it's okay even if it's by accident (dgram didn't grow string support until quite late.)

Applications that send the same string over and over would incur a performance hit because of the string->buffer conversion with every call.

Using util.inspect() inside console.log() is redundant

Yes, if it's the one-argument version.

I have no idea about this one, but is sandbox really optional in script.runInNewContext()?

No, it's not - the documentation is wrong.

Its a lot of work, but I would request you PR each fix individually.

I think a single big PR is acceptable as long as it's broken up in logical commits, i.e., one fix per commit.

[evanlucas]

@davidgilbertson thanks for taking the time to open an issue with all of these!

[sam-github]

@georgiwe not right place, open an issue (or open a PR that improves the docs!).

[Trott]

These appear to have been fixed. Please comment (or re-open) if I am mistaken!