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

Changes to http.md #10614

Closed
wants to merge 1 commit into from
Closed

Changes to http.md #10614

wants to merge 1 commit into from

Conversation

fhalde
Copy link
Contributor

@fhalde fhalde commented Jan 4, 2017

Change KeepAlive to keepAlive
Ref #10567

Checklist
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. http Issues or PRs related to the http subsystem. lts-watch-v6.x labels Jan 4, 2017
@mscdex
Copy link
Contributor

mscdex commented Jan 4, 2017

The HTTP 1.1 RFC actually uses 'Keep-Alive' when referring to the mechanism/functionality in general. I think it would be better to align with the RFC instead.

@Trott
Copy link
Member

Trott commented Jan 4, 2017

For the instances that refer to the flag, keepAlive is right, but use backticks too:

`keepAlive`

That will render like this: keepAlive

For instances that don't refer to the flag but instead refer to HTTP Keep-Alive: Keep-Alive is probably the way to go.

/cc @nodejs/documentation

@fhalde
Copy link
Contributor Author

fhalde commented Jan 4, 2017

@Trott @mscdex everything seems to refer to HTTP Keep-Alive to me 😕
Maybe I should let the one who raised the issue handle this.

@lance
Copy link
Member

lance commented Jan 6, 2017

In the HTTP 1.1 RFC it seems that HTTP Keep-Alive is the mechanism, and Connection: keep-alive is the request header, and in Node, the Agent constructor option is { keepAlive: <boolean>}. There are a few different areas where I think all of this could be clarified. For example:

If you opt into using HTTP KeepAlive, you can create an Agent object with that flag set to true.

Should probably read more like:

If you opt into using HTTP Keep-Alive, you can create an Agent instance, providing { keepAlive: true } among the constructor options.

@fhalde, although I'm not the one who raised the issue initially, I'd be happy to take this on if you're disinclined.

@fhalde
Copy link
Contributor Author

fhalde commented Jan 7, 2017

@lance go ahead dude 😄


If you opt into using HTTP KeepAlive, you can create an Agent object
If you opt into using HTTP keepAlive, you can create an Agent object
Copy link
Contributor

Choose a reason for hiding this comment

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

By the way, the Agent seems to be:

  1. agent in lowercase
  2. Agent with backticks

Copy link
Member

@lance lance Jan 9, 2017

Choose a reason for hiding this comment

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

@yorkie I agree that there are some inconsistencies with Agent as well. Most of the time when we see capital-A, 'Agent' it's directly preceded by "HTTP". As in

The HTTP Agent is used for pooling sockets used in HTTP client requests.

When referred to this way, it seems like it should be referring to "http.Agent" (as code, and scoped to the module), instead of "HTTP Agent" (as a non-standard English language description of the object itself). What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

I guess a follow on question would be whether we should always scope the object name, so that whenever Agent appears in backticks, it is written as http.Agent.

Copy link
Contributor

Choose a reason for hiding this comment

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

@lance I think that's typically only done if you were referencing a module other than the one the current documentation is for.

@lance lance mentioned this pull request Jan 9, 2017
2 tasks
@lance
Copy link
Member

lance commented Jan 9, 2017

I am going to close this in favor of #10715. If that's not the best way to go about it, someone please let me know. :)

@lance lance closed this Jan 9, 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. http Issues or PRs related to the http subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants