From 5bda05548bcfd473b3b112fe388ae64f862af86d Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Sun, 31 Dec 2017 21:14:52 -0800 Subject: [PATCH 01/15] doc: fix typos in CONTRIBUTING.md Remove incorrect usage of "in which". The sentence is better and shorter without it anyway. Replace incorrect "with" with "in". PR-URL: https://github.com/nodejs/node/pull/17930 Reviewed-By: Luigi Pinca Reviewed-By: Weijia Wang Reviewed-By: Jon Moss Reviewed-By: James M Snell Reviewed-By: Colin Ihrig Reviewed-By: Anatoli Papirovski Reviewed-By: Ruben Bridgewater Reviewed-By: Gireesh Punathil Reviewed-By: Yuta Hiroto --- CONTRIBUTING.md | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index d27959c6dc5..1dbaadf6854 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -199,9 +199,8 @@ functional guidelines of the Node.js project. ## Pull Requests -Pull Requests are the way in which concrete changes are made to the code, -documentation, dependencies, and tools contained with the `nodejs/node` -repository. +Pull Requests are the way concrete changes are made to the code, documentation, +dependencies, and tools contained in the `nodejs/node` repository. There are two fundamental components of the Pull Request process: one concrete and technical, and one more process oriented. The concrete and technical From e6a401e0ae01952294f867fbbd715937f8557718 Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Sun, 31 Dec 2017 21:18:44 -0800 Subject: [PATCH 02/15] doc: improve PR-review paragraph in CONTRIBUTING.md * Remove redundant "blocking it or stopping it" as blocking and stopping are the same thing in this case. * Make another sentence less wordy. Fix incorrect verb conjugation. Break into two clear sentences. PR-URL: https://github.com/nodejs/node/pull/17931 Reviewed-By: Luigi Pinca Reviewed-By: Jon Moss Reviewed-By: Joyee Cheung Reviewed-By: James M Snell Reviewed-By: Colin Ihrig Reviewed-By: Ruben Bridgewater Reviewed-By: Gireesh Punathil --- CONTRIBUTING.md | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 1dbaadf6854..b1cc67ada55 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -602,12 +602,11 @@ your name on it. Congratulations and thanks for your contribution! All Node.js contributors who choose to review and provide feedback on Pull Requests have a responsibility to both the project and the individual making the contribution. Reviews and feedback must be helpful, insightful, and geared -towards improving the contribution as opposed to simply blocking it or -stopping it. If there are reasons why you feel the PR should not land, explain -what those are. Do not expect to be able to block a Pull Request from advancing -simply because you say "No" without giving an explanation. It is also important -to be open to having your mind changed, and to being open to working with the -contributor to make the Pull Request better. +towards improving the contribution as opposed to simply blocking it. If there +are reasons why you feel the PR should not land, explain what those are. Do not +expect to be able to block a Pull Request from advancing simply because you say +"No" without giving an explanation. Be open to having your mind changed. Be open +to working with the contributor to make the Pull Request better. Reviews that are dismissive or disrespectful of the contributor or any other reviewers are strictly counter to the [Code of Conduct][]. From 30892c8fb432ac8ee33eae1144bcce704906a855 Mon Sep 17 00:00:00 2001 From: Benjamin Zaslavsky Date: Sun, 24 Dec 2017 09:11:29 +0100 Subject: [PATCH 03/15] test: fix require-deps-deprecation for installed deps Test test-require-deps-deprecation.js was failing when user already had node installed with acorn in require.resolve range. Modified test to acknowledge the possibility and throw only if acorn is found in the deps directory. PR-URL: https://github.com/nodejs/node/pull/17848 Fixes: https://github.com/nodejs/node/issues/17148 Reviewed-By: Tiancheng "Timothy" Gu Reviewed-By: Colin Ihrig Reviewed-By: Rich Trott Reviewed-By: James M Snell --- test/parallel/test-require-deps-deprecation.js | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/test/parallel/test-require-deps-deprecation.js b/test/parallel/test-require-deps-deprecation.js index ebb31f6464f..b588fa4da9e 100644 --- a/test/parallel/test-require-deps-deprecation.js +++ b/test/parallel/test-require-deps-deprecation.js @@ -39,6 +39,17 @@ for (const m of deprecatedModules) { } catch (err) {} } +// Instead of checking require, check that resolve isn't pointing toward a +// built-in module, as user might already have node installed with acorn in +// require.resolve range. +// Ref: https://github.com/nodejs/node/issues/17148 for (const m of deps) { - assert.throws(() => { require(m); }, /^Error: Cannot find module/); + let path; + try { + path = require.resolve(m); + } catch (err) { + assert.ok(err.toString().startsWith('Error: Cannot find module ')); + continue; + } + assert.notStrictEqual(path, m); } From b25b1efa0a6801cdf3faea199a36e0e2cc489395 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Mon, 1 Jan 2018 11:13:05 -0800 Subject: [PATCH 04/15] tls: set servername on client side too PR-URL: https://github.com/nodejs/node/pull/17935 Reviewed-By: Anatoli Papirovski Reviewed-By: Sebastiaan Deckers Reviewed-By: Tiancheng "Timothy" Gu --- lib/_tls_wrap.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/_tls_wrap.js b/lib/_tls_wrap.js index 32fd1f322e0..bb25eb5f155 100644 --- a/lib/_tls_wrap.js +++ b/lib/_tls_wrap.js @@ -621,7 +621,7 @@ TLSSocket.prototype._finishInit = function() { this.alpnProtocol = this._handle.getALPNNegotiatedProtocol(); } - if (process.features.tls_sni && this._tlsOptions.isServer) { + if (process.features.tls_sni) { this.servername = this._handle.getServername(); } From 060babd66524b6a3e4757bb2fe5b87ad567cdb40 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Mon, 1 Jan 2018 11:13:29 -0800 Subject: [PATCH 05/15] http2: add initial support for originSet Add new properties to `Http2Session` to identify alpnProtocol, and indicator about whether the session is TLS or not, and initial support for origin set (preparinng for `ORIGIN` frame support and the client-side `Pool` implementation. The `originSet` is the set of origins for which an `Http2Session` may be considered authoritative. Per the `ORIGIN` frame spec, the originSet is only valid on TLS connections, so this is only exposed when using a `TLSSocket`. PR-URL: https://github.com/nodejs/node/pull/17935 Reviewed-By: Anatoli Papirovski Reviewed-By: Sebastiaan Deckers Reviewed-By: Tiancheng "Timothy" Gu --- doc/api/http2.md | 35 +++++++++++ lib/internal/http2/core.js | 58 ++++++++++++++++++- ...test-http2-create-client-secure-session.js | 19 ++++++ .../test-http2-create-client-session.js | 8 ++- 4 files changed, 118 insertions(+), 2 deletions(-) diff --git a/doc/api/http2.md b/doc/api/http2.md index e9b06a427c2..f6130723241 100644 --- a/doc/api/http2.md +++ b/doc/api/http2.md @@ -283,6 +283,18 @@ session.setTimeout(2000); session.on('timeout', () => { /** .. **/ }); ``` +#### http2session.alpnProtocol + + +* Value: {string|undefined} + +Value will be `undefined` if the `Http2Session` is not yet connected to a +socket, `h2c` if the `Http2Session` is not connected to a `TLSSocket`, or +will return the value of the connected `TLSSocket`'s own `alpnProtocol` +property. + #### http2session.close([callback]) + +* Value: {boolean|undefined} + +Value is `undefined` if the `Http2Session` session socket has not yet been +connected, `true` if the `Http2Session` is connected with a `TLSSocket`, +and `false` if the `Http2Session` is connected to any other kind of socket +or stream. + #### http2session.goaway([code, [lastStreamID, [opaqueData]]]) + +* Value: {string[]|undefined} + +If the `Http2Session` is connected to a `TLSSocket`, the `originSet` property +will return an Array of origins for which the `Http2Session` may be +considered authoritative. + #### http2session.pendingSettingsAck + + + +```js +const http2 = require('../common/http2'); +``` + +### Class: Frame + +The `http2.Frame` is a base class that creates a `Buffer` containing a +serialized HTTP/2 frame header. + + + + + +```js +// length is a 24-bit unsigned integer +// type is an 8-bit unsigned integer identifying the frame type +// flags is an 8-bit unsigned integer containing the flag bits +// id is the 32-bit stream identifier, if any. +const frame = new http2.Frame(length, type, flags, id); + +// Write the frame data to a socket +socket.write(frame.data); +``` + +The serialized `Buffer` may be retrieved using the `frame.data` property. + +### Class: DataFrame extends Frame + +The `http2.DataFrame` is a subclass of `http2.Frame` that serializes a `DATA` +frame. + + + + + +```js +// id is the 32-bit stream identifier +// payload is a Buffer containing the DATA payload +// padlen is an 8-bit integer giving the number of padding bytes to include +// final is a boolean indicating whether the End-of-stream flag should be set, +// defaults to false. +const data = new http2.DataFrame(id, payload, padlen, final); + +socket.write(frame.data); +``` + +### Class: HeadersFrame + +The `http2.HeadersFrame` is a subclass of `http2.Frame` that serializes a +`HEADERS` frame. + + + + + +```js +// id is the 32-bit stream identifier +// payload is a Buffer containing the HEADERS payload (see either +// http2.kFakeRequestHeaders or http2.kFakeResponseHeaders). +// padlen is an 8-bit integer giving the number of padding bytes to include +// final is a boolean indicating whether the End-of-stream flag should be set, +// defaults to false. +const data = new http2.HeadersFrame(id, http2.kFakeRequestHeaders, + padlen, final); + +socket.write(frame.data); +``` + +### Class: SettingsFrame + +The `http2.SettingsFrame` is a subclass of `http2.Frame` that serializes an +empty `SETTINGS` frame. + + + + + +```js +// ack is a boolean indicating whether or not to set the ACK flag. +const frame = new http2.SettingsFrame(ack); + +socket.write(frame.data); +``` + +### http2.kFakeRequestHeaders + +Set to a `Buffer` instance that contains a minimal set of serialized HTTP/2 +request headers to be used as the payload of a `http2.HeadersFrame`. + + + + + +```js +const frame = new http2.HeadersFrame(1, http2.kFakeRequestHeaders, 0, true); + +socket.write(frame.data); +``` + +### http2.kFakeResponseHeaders + +Set to a `Buffer` instance that contains a minimal set of serialized HTTP/2 +response headers to be used as the payload a `http2.HeadersFrame`. + + + + + +```js +const frame = new http2.HeadersFrame(1, http2.kFakeResponseHeaders, 0, true); + +socket.write(frame.data); +``` + +### http2.kClientMagic + +Set to a `Buffer` containing the preamble bytes an HTTP/2 client must send +upon initial establishment of a connection. + + + + + +```js +socket.write(http2.kClientMagic); +``` + + [<Array>]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array [<ArrayBufferView[]>]: https://developer.mozilla.org/en-US/docs/Web/API/ArrayBufferView [<Boolean>]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Data_structures#Boolean_type From ecf6e79362faad747f50405d7f36a5d5006c3e97 Mon Sep 17 00:00:00 2001 From: Gibson Fahnestock Date: Thu, 28 Dec 2017 13:29:36 +0200 Subject: [PATCH 09/15] doc: remove x86 from os.arch() options It is not possible for `process.arch` (which comes from V8's `target_arch`) to be `x86`. Also updates `process.arch` to have the same information. PR-URL: https://github.com/nodejs/node/pull/17899 Reviewed-By: Ben Noordhuis Reviewed-By: Luigi Pinca Reviewed-By: Anatoli Papirovski Reviewed-By: James M Snell Reviewed-By: Colin Ihrig Reviewed-By: Tiancheng "Timothy" Gu Reviewed-By: Daniel Bevenius --- doc/api/os.md | 5 ++--- doc/api/process.md | 8 +++++--- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/doc/api/os.md b/doc/api/os.md index e77238021c6..bb25c3f6cc2 100644 --- a/doc/api/os.md +++ b/doc/api/os.md @@ -31,11 +31,10 @@ added: v0.5.0 * Returns: {string} The `os.arch()` method returns a string identifying the operating system CPU -architecture *for which the Node.js binary was compiled*. +architecture for which the Node.js binary was compiled. The current possible values are: `'arm'`, `'arm64'`, `'ia32'`, `'mips'`, -`'mipsel'`, `'ppc'`, `'ppc64'`, `'s390'`, `'s390x'`, `'x32'`, `'x64'`, and -`'x86'`. +`'mipsel'`, `'ppc'`, `'ppc64'`, `'s390'`, `'s390x'`, `'x32'`, and `'x64'`. Equivalent to [`process.arch`][]. diff --git a/doc/api/process.md b/doc/api/process.md index 60545ff0628..2eecdc832af 100644 --- a/doc/api/process.md +++ b/doc/api/process.md @@ -432,9 +432,11 @@ added: v0.5.0 * {string} -The `process.arch` property returns a String identifying the processor -architecture that the Node.js process is currently running on. For instance -`'arm'`, `'ia32'`, or `'x64'`. +The `process.arch` property returns a string identifying the operating system CPU +architecture for which the Node.js binary was compiled. + +The current possible values are: `'arm'`, `'arm64'`, `'ia32'`, `'mips'`, +`'mipsel'`, `'ppc'`, `'ppc64'`, `'s390'`, `'s390x'`, `'x32'`, and `'x64'`. ```js console.log(`This processor architecture is ${process.arch}`); From 26607b825ebebe78a182951af164fd1e319dfe27 Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Wed, 27 Dec 2017 19:21:06 -0800 Subject: [PATCH 10/15] doc: edit for concision This removes some wordy phrases (notably "it is important to note that"). PR-URL: https://github.com/nodejs/node/pull/17891 Reviewed-By: Gibson Fahnestock Reviewed-By: Colin Ihrig Reviewed-By: Joyee Cheung --- doc/api/async_hooks.md | 11 +++++------ doc/api/errors.md | 9 ++++----- doc/api/process.md | 19 +++++++------------ doc/api/repl.md | 5 ++--- doc/api/stream.md | 11 +++++------ doc/api/vm.md | 5 ++--- 6 files changed, 25 insertions(+), 35 deletions(-) diff --git a/doc/api/async_hooks.md b/doc/api/async_hooks.md index 7d07622eda7..e8cb9344c4c 100644 --- a/doc/api/async_hooks.md +++ b/doc/api/async_hooks.md @@ -484,9 +484,8 @@ fs.open(path, 'r', (err, fd) => { }); ``` -It is important to note that the ID returned fom `executionAsyncId()` is related -to execution timing, not causality (which is covered by `triggerAsyncId()`). For -example: +The ID returned fom `executionAsyncId()` is related to execution timing, not +causality (which is covered by `triggerAsyncId()`). For example: ```js const server = net.createServer(function onConnection(conn) { @@ -538,9 +537,9 @@ own resources. The `init` hook will trigger when an `AsyncResource` is instantiated. -*Note*: It is important that `before`/`after` calls are unwound -in the same order they are called. Otherwise an unrecoverable exception -will occur and the process will abort. +*Note*: `before` and `after` calls must be unwound in the same order that they +are called. Otherwise, an unrecoverable exception will occur and the process +will abort. The following is an overview of the `AsyncResource` API. diff --git a/doc/api/errors.md b/doc/api/errors.md index 1a45bbe6e0d..d199e3affd7 100644 --- a/doc/api/errors.md +++ b/doc/api/errors.md @@ -312,11 +312,10 @@ location information will be displayed for that frame. Otherwise, the determined function name will be displayed with location information appended in parentheses. -It is important to note that frames are **only** generated for JavaScript -functions. If, for example, execution synchronously passes through a C++ addon -function called `cheetahify`, which itself calls a JavaScript function, the -frame representing the `cheetahify` call will **not** be present in the stack -traces: +Frames are only generated for JavaScript functions. If, for example, execution +synchronously passes through a C++ addon function called `cheetahify` which +itself calls a JavaScript function, the frame representing the `cheetahify` call +will not be present in the stack traces: ```js const cheetahify = require('./native-binding.node'); diff --git a/doc/api/process.md b/doc/api/process.md index 2eecdc832af..b9832bc1ff1 100644 --- a/doc/api/process.md +++ b/doc/api/process.md @@ -374,16 +374,11 @@ process.on('SIGINT', handle); process.on('SIGTERM', handle); ``` -*Note*: An easy way to send the `SIGINT` signal is with `-C` in most -terminal programs. - -It is important to take note of the following: - * `SIGUSR1` is reserved by Node.js to start the [debugger][]. It's possible to install a listener but doing so will _not_ stop the debugger from starting. * `SIGTERM` and `SIGINT` have default handlers on non-Windows platforms that - resets the terminal mode before exiting with code `128 + signal number`. If - one of these signals has a listener installed, its default behavior will be + reset the terminal mode before exiting with code `128 + signal number`. If one + of these signals has a listener installed, its default behavior will be removed (Node.js will no longer exit). * `SIGPIPE` is ignored by default. It can have a listener installed. * `SIGHUP` is generated on Windows when the console window is closed, and on @@ -394,7 +389,7 @@ It is important to take note of the following: installed its default behavior will be removed. * `SIGTERM` is not supported on Windows, it can be listened on. * `SIGINT` from the terminal is supported on all platforms, and can usually be - generated with `CTRL+C` (though this may be configurable). It is not generated + generated with `+C` (though this may be configurable). It is not generated when terminal raw mode is enabled. * `SIGBREAK` is delivered on Windows when `+` is pressed, on non-Windows platforms it can be listened on, but there is no way to send or @@ -989,10 +984,10 @@ process.exit(1); The shell that executed Node.js should see the exit code as `1`. -It is important to note that calling `process.exit()` will force the process to -exit as quickly as possible *even if there are still asynchronous operations -pending* that have not yet completed fully, *including* I/O operations to -`process.stdout` and `process.stderr`. +Calling `process.exit()` will force the process to exit as quickly as possible +even if there are still asynchronous operations pending that have not yet +completed fully, including I/O operations to `process.stdout` and +`process.stderr`. In most situations, it is not actually necessary to call `process.exit()` explicitly. The Node.js process will exit on its own *if there is no additional diff --git a/doc/api/repl.md b/doc/api/repl.md index 56661d86d9f..b71ed111b42 100644 --- a/doc/api/repl.md +++ b/doc/api/repl.md @@ -114,9 +114,8 @@ $ node repl_test.js 'message' ``` -It is important to note that context properties are *not* read-only by default. -To specify read-only globals, context properties must be defined using -`Object.defineProperty()`: +Context properties are not read-only by default. To specify read-only globals, +context properties must be defined using `Object.defineProperty()`: ```js const repl = require('repl'); diff --git a/doc/api/stream.md b/doc/api/stream.md index 741f1054642..10bd9515051 100644 --- a/doc/api/stream.md +++ b/doc/api/stream.md @@ -1426,12 +1426,11 @@ successfully or failed with an error. The first argument passed to the `callback` must be the `Error` object if the call failed or `null` if the write succeeded. -It is important to note that all calls to `writable.write()` that occur between -the time `writable._write()` is called and the `callback` is called will cause -the written data to be buffered. Once the `callback` is invoked, the stream will -emit a [`'drain'`][] event. If a stream implementation is capable of processing -multiple chunks of data at once, the `writable._writev()` method should be -implemented. +All calls to `writable.write()` that occur between the time `writable._write()` +is called and the `callback` is called will cause the written data to be +buffered. Once the `callback` is invoked, the stream will emit a [`'drain'`][] +event. If a stream implementation is capable of processing multiple chunks of +data at once, the `writable._writev()` method should be implemented. If the `decodeStrings` property is set in the constructor options, then `chunk` may be a string rather than a Buffer, and `encoding` will diff --git a/doc/api/vm.md b/doc/api/vm.md index d700e6344f5..3ac70091e58 100644 --- a/doc/api/vm.md +++ b/doc/api/vm.md @@ -87,9 +87,8 @@ changes: depending on whether code cache data is produced successfully. Creating a new `vm.Script` object compiles `code` but does not run it. The -compiled `vm.Script` can be run later multiple times. It is important to note -that the `code` is not bound to any global object; rather, it is bound before -each run, just for that run. +compiled `vm.Script` can be run later multiple times. The `code` is not bound to +any global object; rather, it is bound before each run, just for that run. ### script.runInContext(contextifiedSandbox[, options])