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: fix mistake in http2stream.respondWithFile. #15501

Conversation

antoine-amara
Copy link
Contributor

@antoine-amara antoine-amara commented Sep 20, 2017

http2stream.respondWithFile api has changed since 8.5.0 with the
addition of the onError option. In the first code example an
onError function is implemented but never used, fix this
mistake.

Add a description more informations when onError is triggered.

Fixes: #15390

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. http2 Issues or PRs related to the http2 subsystem. labels Sep 20, 2017
doc/api/http2.md Outdated
@@ -1146,7 +1146,8 @@ of the given file:

If an error occurs while attempting to read the file data, the `Http2Stream`
will be closed using an `RST_STREAM` frame using the standard `INTERNAL_ERROR`
code.
code. The onError callback will be called before send if is defined, otherwise
Copy link
Member

@lpinca lpinca Sep 21, 2017

Choose a reason for hiding this comment

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

Nit: can you please add backticks (`) around onError?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Of course, I will fix that and I rebase on master too.

@antoine-amara antoine-amara force-pushed the doc-#15390-update-responseWithFile branch from 3959dc9 to 705bbe2 Compare September 21, 2017 11:34
@antoine-amara
Copy link
Contributor Author

Fix the onError syntax and rebase the branch on master. Thanks for the review.

doc/api/http2.md Outdated
@@ -1147,7 +1147,8 @@ of the given file:

If an error occurs while attempting to read the file data, the `Http2Stream`
will be closed using an `RST_STREAM` frame using the standard `INTERNAL_ERROR`
code.
code. The `onError` callback will be called before send if is defined, otherwise
Copy link
Member

Choose a reason for hiding this comment

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

The send if is defined sounds somewhat wrong to me. Can we change that to something like
If the `onError` callback is defined it will be called, otherwise the stream will be destroyed.?

@antoine-amara antoine-amara force-pushed the doc-#15390-update-responseWithFile branch from 705bbe2 to 2200309 Compare September 24, 2017 17:56
http2stream.respondWithFile api has changed since 8.5.0 with the
addition of the onError option. In the first code example an
onError function is implemented but never used, fix this
mistake.

Add a description to have more informations when onError is triggered.

Fixes: nodejs#15390
@antoine-amara antoine-amara force-pushed the doc-#15390-update-responseWithFile branch from 2200309 to 42c244d Compare September 26, 2017 11:15
@BridgeAR
Copy link
Member

Landed in 51bc7fa

@BridgeAR BridgeAR closed this Sep 27, 2017
BridgeAR pushed a commit that referenced this pull request Sep 27, 2017
http2stream.respondWithFile api has changed since 8.5.0 with the
addition of the onError option. In the first code example an
onError function is implemented but never used, fix this
mistake.

Add a description to have more informations when onError is triggered.

PR-URL: #15501
Fixes: #15390
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Sep 28, 2017
http2stream.respondWithFile api has changed since 8.5.0 with the
addition of the onError option. In the first code example an
onError function is implemented but never used, fix this
mistake.

Add a description to have more informations when onError is triggered.

PR-URL: nodejs#15501
Fixes: nodejs#15390
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@antoine-amara antoine-amara deleted the doc-#15390-update-responseWithFile branch September 28, 2017 08:08
MylesBorins pushed a commit that referenced this pull request Sep 29, 2017
http2stream.respondWithFile api has changed since 8.5.0 with the
addition of the onError option. In the first code example an
onError function is implemented but never used, fix this
mistake.

Add a description to have more informations when onError is triggered.

PR-URL: #15501
Fixes: #15390
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
addaleax pushed a commit to addaleax/ayo that referenced this pull request Sep 30, 2017
http2stream.respondWithFile api has changed since 8.5.0 with the
addition of the onError option. In the first code example an
onError function is implemented but never used, fix this
mistake.

Add a description to have more informations when onError is triggered.

PR-URL: nodejs/node#15501
Fixes: nodejs/node#15390
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MylesBorins pushed a commit that referenced this pull request Oct 3, 2017
http2stream.respondWithFile api has changed since 8.5.0 with the
addition of the onError option. In the first code example an
onError function is implemented but never used, fix this
mistake.

Add a description to have more informations when onError is triggered.

PR-URL: #15501
Fixes: #15390
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Oct 3, 2017
MylesBorins pushed a commit that referenced this pull request Oct 3, 2017
http2stream.respondWithFile api has changed since 8.5.0 with the
addition of the onError option. In the first code example an
onError function is implemented but never used, fix this
mistake.

Add a description to have more informations when onError is triggered.

PR-URL: #15501
Fixes: #15390
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
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. http2 Issues or PRs related to the http2 subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

http2: Change in respondWithFile api not properly reflected in the docs
7 participants