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

doc: improve https module documentation #7847

Closed
wants to merge 1 commit into from

Conversation

mscdex
Copy link
Contributor

@mscdex mscdex commented Jul 23, 2016

Checklist
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)
  • doc
Description of change

This commit makes the following changes:

  • Reduces redundancy
  • Highlights class inheritance
  • Adds function descriptions
  • Uses consistent language and formatting in text and consistent coding style in examples

@mscdex mscdex added https Issues or PRs related to the https subsystem. doc Issues and PRs related to the documentations. labels Jul 23, 2016
@mscdex
Copy link
Contributor Author

mscdex commented Jul 23, 2016

/cc @nodejs/documentation @nodejs/collaborators

This commit makes the following changes:
* Reduces redundancy
* Highlights class inheritance
* Adds function descriptions
* Uses consistent language and formatting in text
  and consistent coding style in examples

## https.createServer(options[, requestListener])
## https.createServer(options[, callback])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is requestListener in http docs, why change it here?

Copy link
Contributor Author

@mscdex mscdex Jul 23, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Eventual) Consistency? I am working on it all one module at a time.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So is it going to be changed in http.md after this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that is my plan. I'm actually working on http.md right now.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry then! LGTM

@indutny
Copy link
Member

indutny commented Jul 23, 2016

One nit, otherwise LGTM.

This class is a subclass of `tls.Server` and emits events same as
[`http.Server`][]. See [`http.Server`][] for more information.
**Inherits:** [`tls.Server`]
**Implements:** [`http.Server`]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should be some explanation for this implements. Inherits and implements is kinda confusing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm open to suggestions on naming, formatting, etc. The goal here is to document inheritance and the like, which also helps reduce redundant documentation.

I just thought of something that would be neat, but is out of the scope of what I'm trying to do at the moment: if we could (dynamically) integrate collapsed (by default) sections that would show inherited events, methods, and properties (one collapsed section per type). That would help prevent having to navigate to a separate page to see inherited API while still leaving the UI less cluttered and easy to see API unique to that object.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been considering this also but it's going to be a bit difficult given how we currently manage the documentation. I've been thinking about a more radical approach where we break the documentation up a bit more in order to make individual APIs easier to document and modify. I think, for now, however, this is a good change as is.

@mcollina
Copy link
Member

I'm 👎 on reducing redundancy. I've found out that a lot of developers have a rough understanding of how TLS works, how self-signed certificate works, how to set them up and the like.
In https://github.com/mqttjs/MQTT.js#mqttclientstreambuilder-options we are linking to tls.connect(), and we are getting a huge number of questions related to those options, and how to use them. I like the fact that all the options for 'tls' are in 'https' as well.

Still, this PR is great from a maintenance point of view or if you know how TLS works. Most people do not want to get into TLS plumbing, the just want a quick primer on how to use the 'https' module.

@mscdex
Copy link
Contributor Author

mscdex commented Jul 25, 2016

@mcollina I don't understand the problem when we have a link to both http.request() and tls.connect() together there in the description. Are you saying most users can't be bothered to click another link?

@mcollina
Copy link
Member

@mscdex I have seen people struggling to grasp both http and tls at the same, and they have to retain multiple conceptual layers which is currently unnecessary. It's about cognitive context switching.

The current docs serve both as a reference and as a way to learn a topic. Removing the duplication will retain the function as a reference, but will damper the function as learning manual.

@mscdex
Copy link
Contributor Author

mscdex commented Jul 25, 2016

Well, I'm open to finding other ways to reduce physical redundancy. If there is some clever way to pull in the list of options from tls.connect() on-the-fly or something, then that's fine too I guess. IMHO the duplicating of documentation can lead to inconsistencies. I am already seeing instances of this while reviewing the documentation for various modules.

@jasnell
Copy link
Member

jasnell commented Jul 26, 2016

While the current documentation is a bit limited and can be confusing, I think the key will be in making incremental steps towards improving rather than making significant sweeping changes all at once. These changes LGTM with a couple of nits.

@mcollina
Copy link
Member

@mscdex can you please add a link on what a self-signed certificate is and how to set that up? Something related to rejectUnauthorized and DEPTH_ZERO_SELF_SIGNED_CERT.

@jasnell
Copy link
Member

jasnell commented Jan 6, 2017

Ping @mscdex ... what do you want to do with this?

@jasnell jasnell added the stalled Issues and PRs that are stalled. label Jan 6, 2017
@mscdex
Copy link
Contributor Author

mscdex commented Jan 6, 2017

I'm still interested in landing it, I just haven't had time yet to dive back into the docs again.

for more information.
**Inherits:** [`http.Agent`]

An Agent object for HTTPS.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

small nit: Agent should be in lowercase?

@jasnell
Copy link
Member

jasnell commented Aug 24, 2017

Closing due to lack of any forward progress on this. @mscdex ... please reopen if you wish to keep pursuing it.

@jasnell jasnell closed this Aug 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. https Issues or PRs related to the https subsystem. stalled Issues and PRs that are stalled.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants