v11.7.0 proposal - #25537
Merged
BridgeAR merged 246 commits intoJan 18, 2019
Merged
Conversation
Copy link
Copy Markdown
Member
Sorry, something went wrong.
In 180f865, the test was changed so that the `env` argument of `createInternalRepl()` also contained external environment variables, because keeping them can be necessary for spawning processes on some systems. However, this test does not spawn new processes, and relies on the fact that the environment variables it tests are not already set (and fails otherwise); therefore, reverting to the original state should fix this. Fixes: nodejs#21451 Fixes: nodejs/build#1377 Refs: nodejs#25219 PR-URL: nodejs#25226 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Denys Otrishko <shishugi@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
PR-URL: nodejs#25216 Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Remove unused `name` argument that is different from a subsequent `name` argument in a different but nearby function. This was mildly confusing to me at first, so hopefully this change clarifies things for others reading the test. PR-URL: nodejs#25117 Reviewed-By: James M Snell <jasnell@gmail.com>
'close' event isn't emitted on a TLS connection if it's been written to (but 'end' and 'finish' events are). PR-URL: nodejs#25026 Fixes: nodejs#24984 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
Instead of using an undocumented underscore-prefixed property to configure the writable stream instance to not encode strings as buffers, use the public API which is an options property passed to the constructor. Refs: nodejs#445 PR-URL: nodejs#25201 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
We were previously reading from the wrong offset, namely the one into the final results array, not the one for the AAAA results itself, which could have lead to reading uninitialized or out-of-bounds data. Also, adjust the test accordingly; TTL values are not modified by c-ares, but are only exposed for a subset of all DNS record types. PR-URL: nodejs#25187 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Khaidi Chu <i@2333.moe> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
This would otherwise be reported as a memory leak by automated tools. PR-URL: nodejs#25180 Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
During the time between nodejs#24234 being opened and it landing, a V8 update occurred that deprecated several APIs. This commit fixes the following compiler warnings: ../src/node_crypto.cc:3342:11: warning: 'Set' is deprecated: Use maybe version ../src/node_crypto.cc:3345:13: warning: 'GetFunction' is deprecated: Use maybe version PR-URL: nodejs#25205 Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Revise the "Breaking Changes to Internal Elements" section of COLLABORATOR_GUIDE.md. Simplify wording, shorten sentences, and remove redundant material. PR-URL: nodejs#25190 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
PR-URL: nodejs#25207 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
This commit adds a DCHECK macro for consistency with the other DCHECK_* macros. PR-URL: nodejs#25207 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Set encoding on the stderr/stdout streams instead of calling data.toString(). Don't assume the complete expected messages arrive in a single event. PR-URL: nodejs#25169 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
This adds support for inspect to distinguish regular expression subclasses and ones with null prototype from "normal" regular expressions. PR-URL: nodejs#25192 Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
So far we do not test all data types for subclasses and this extends the existing tests for WeakSet, WeakMap and BigInt64Array. PR-URL: nodejs#25192 Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: nodejs#25149 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Simplify code by using return value of Object.defineProperty directly. PR-URL: nodejs#25162 Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com> Reviewed-By: Masashi Hirano <shisama07@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Link to the cli docs for --tls-cipher-list, and link to and mention that NODE_OPTIONS can also be used. PR-URL: nodejs#25174 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
PR-URL: nodejs#25174 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
sequential/test-inspector-debug-end and parallel/test-child-process-execfile Off late these have been failing in AIX. Debugging core dump suggested that this is a side effect of exit-race that is described in nodejs#25007 Mart these as flaky in AIX until that is resolved. Refs: nodejs#25047 Refs: nodejs#25029 PR-URL: nodejs#25126 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
This commit simplifies the calling of normalizeSpawnArguments() and normalizeExecArguments(). Specifically, this commit replaces apply() and the use of arguments with a normal function call. PR-URL: nodejs#25194 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
The OCSP info from parsing the TLS ClientHello has not been used since 550c263, remove it. See: nodejs#1464 PR-URL: nodejs#25153 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
"wrapped" argument is the caller's "socket", not its "wrap", and its referred to as "socket" in the comments, so call it that. PR-URL: nodejs#25153 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
session ID was named session in C++ and key in JS, Name them after what they are, as the 'newSession' event docs do. PR-URL: nodejs#25153 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Its confusing to call a js class with a handle a "Wrap", usually it's the C++ handle that is called a Wrap (tcp_wrap, tls_wrap, ...). Its derived from Socket, and makes a JS stream look like a Socket, so call it that. Also, remove use of lib/_stream_wrap.js so it can be deprecated some time. PR-URL: nodejs#25153 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Split test-whatwg-encoding-textdecoder.js into: - `test-whatwg-encoding-custom-textdecoder.js` which tests Node.js-specific behaviors - `test-whatwg-encoding-custom-textdecoder-api-invalid-label.js` which is a customized version of the WPT counterpart - `test-whatwg-encoding-custom-api-basics.js` which is the part of `test-whatwg-encoding-api-basics.js` that can be run without ICU - `test-whatwg-encoding-api-basics.js` which can be replaced with WPT later. PR-URL: nodejs#25155 Reviewed-By: James M Snell <jasnell@gmail.com>
Split `test-whatwg-encoding-textdecoder-fatal.js` into - `test-whatwg-encoding-custom-textdecoder-fatal.js` which is a customized version of the WPT that tests for Node.js-specific error codes. - `test-whatwg-encoding-custom-textdecoder-invalid-arg` which tests `ERR_INVALID_ARG_TYPE` PR-URL: nodejs#25155 Reviewed-By: James M Snell <jasnell@gmail.com>
Split the following tests: - `test-whatwg-encoding-textdecoder-utf16-surrogates.js` - `test-whatwg-encoding-textdecoder-ignorebom.js` - `test-whatwg-encoding-textdecoder-streaming.js` Each into two files: one that can be run without ICU and one that has to be run with ICU. The latter can be replaced with WPT later. PR-URL: nodejs#25155 Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: nodejs#25155 Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: nodejs#25155 Reviewed-By: James M Snell <jasnell@gmail.com>
This removes the `internalBinding('uv')` call from the normal
bootstrap for now, and avoids building `errmap` by default which
expands to a lot of calls into V8.
PR-URL: nodejs#25143
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Copy link
Copy Markdown
Member
Author
Sorry, something went wrong.
Copy link
Copy Markdown
Member
Author
|
There has been no argument about #22478, so I kept that commit. |
Sorry, something went wrong.
Copy link
Copy Markdown
Member
Author
|
CITGM looked fine, the CI was green (running the CI again after the last change should not be necessary). Running the V8 CI again before publishing this as release: https://ci.nodejs.org/job/node-test-commit-v8-linux/2018/ |
Sorry, something went wrong.
BridgeAR
force-pushed
the
v11.7.0-proposal
branch
from
03f3361 to
b408aa5
Compare
January 17, 2019 17:14
Copy link
Copy Markdown
Member
I think that was before our whitelisting approach for process.binding(), so this is no longer something of concern. 👍 Fwiw, I’d also consider #25361 to be a notable change. |
Sorry, something went wrong.
Copy link
Copy Markdown
Member
Author
|
A single V8 test failed continuously: v8tests.mjsunit/wasm/jsapi-harness It seemed like it also failed on v11.6.1 (see https://ci.nodejs.org/job/node-test-commit-v8-linux/2020/testReport/) so I do not consider this blocking but we should have a closer look at that again. |
Sorry, something went wrong.
Notable Changes
* compression / zlib:
* Added brotli support (Anna Henningsen and Zach Vacura)
nodejs#24938
* console:
* Added `inspectOptions` option (Ruben Bridgewater)
nodejs#24978
* crypto:
* Always accept private keys as public keys (Tobias Nießen)
nodejs#25217
* deps:
* Upgrade npm to v6.5.0 (Jordan Harband)
nodejs#25234
* fs:
* Use internalBinding('fs') internally instead of
process.binding('fs') (Masashi Hirano)
nodejs#22478
* http(s):
* Support overriding http\\s.globalAgent (Roy Sommer)
nodejs#25170
* util:
* Inspect ArrayBuffers contents closely (Ruben Bridgewater)
nodejs#25006
* worker:
* Expose workers by default and remove `--experimental-worker` flag
(Anna Henningsen) nodejs#25361
PR-URL: nodejs#25537
BridgeAR
force-pushed
the
v11.7.0-proposal
branch
from
b408aa5 to
6b7b827
Compare
January 17, 2019 19:51
Copy link
Copy Markdown
Member
Author
|
I added #25361 as notable change. |
Sorry, something went wrong.
PR-URL: nodejs#25537
Copy link
Copy Markdown
Contributor
|
why wasn't #22478 simply rebased off staging? Not blocking on that but confused on why it needs to exist on the release line. |
Sorry, something went wrong.
Copy link
Copy Markdown
Member
|
Can we at least remove the **(SEMVER-MAJOR)** labels from the changelog? |
Sorry, something went wrong.
Copy link
Copy Markdown
Contributor
|
Was just reviewing CITGM results and it seems like the node-report failure is legitimate. |
Sorry, something went wrong.
Copy link
Copy Markdown
Contributor
|
it's --> #25258 specifically d718625 Can confirm that reverting the above commit fixes node-reports's test suite /cc @gabrielf @addaleax @rnchamberlain @richardlau error: | not ok 9 - test/test-api.js # time=231.79ms error: | --- error: | timeout: 300000 error: | file: test/test-api.js error: | results: error: | ok: false error: | count: 3 error: | pass: 2 error: | fail: 1 error: | plan: error: | start: 1 error: | end: 3 error: | exitCode: 1 error: | command: /usr/local/bin/node error: | arguments: error: | - test/test-api.js error: | ... |
Sorry, something went wrong.
Copy link
Copy Markdown
Member
|
@MylesBorins is it only in v11.x-staging or is node-report also broken with master? |
Sorry, something went wrong.
Copy link
Copy Markdown
Contributor
|
@targos looking into that right now. Trying to make a reproduction but it looks like the failure is related to a native module. here's where I'm at right now with a test // node-report
if (process.argv[2] === 'child') {
const api = require('../api');
api.setEvents('signal');
} else {
const assert = require('assert');
const spawn = require('child_process').spawn;
const child = spawn(process.execPath, [__filename, 'child']);
child.on('exit', (code) => {
assert.strictEqual(code, 0, `process exited with code: ${code}`);
});
}if we don't setEvents('signal') the below failure does not happen. api is a native module assert.js:86
throw new AssertionError(obj);
^
AssertionError [ERR_ASSERTION]: process exited with code: null
at ChildProcess.child.on (/Users/mylesborins/code/node-report/test/test-api.js:10:12)
at ChildProcess.emit (events.js:188:13)
at Process.ChildProcess._handle.onexit (internal/child_process.js:254:12)
edit: here is the SetEvents method https://github.com/nodejs/node-report/blob/master/src/module.cc#L103-L134 |
Sorry, something went wrong.
Copy link
Copy Markdown
Member
|
It would be great if #25562 could make it into the release. |
Sorry, something went wrong.
BridgeAR
added a commit
that referenced
this pull request
Jan 18, 2019
Notable Changes
* compression / zlib:
* Added brotli support (Anna Henningsen and Zach Vacura)
#24938
* console:
* Added `inspectOptions` option (Ruben Bridgewater)
#24978
* crypto:
* Always accept private keys as public keys (Tobias Nießen)
#25217
* deps:
* Upgrade npm to v6.5.0 (Jordan Harband)
#25234
* fs:
* Use internalBinding('fs') internally instead of
process.binding('fs') (Masashi Hirano)
#22478
* http(s):
* Support overriding http\\s.globalAgent (Roy Sommer)
#25170
* util:
* Inspect ArrayBuffers contents closely (Ruben Bridgewater)
#25006
* worker:
* Expose workers by default and remove `--experimental-worker` flag
(Anna Henningsen) #25361
PR-URL: #25537
Copy link
Copy Markdown
Member
Author
|
I could not remove the mentioned commit anymore as I had a signed tag for the release before. I just released this as is and I am preparing a patch release now. |
Sorry, something went wrong.
BridgeAR
added a commit
to BridgeAR/nodejs.org
that referenced
this pull request
Jan 18, 2019
BridgeAR
added a commit
to nodejs/nodejs.org
that referenced
this pull request
Jan 18, 2019
Copy link
Copy Markdown
Contributor
|
Please remove both commits from the offending pr |
Sorry, something went wrong.
Copy link
Copy Markdown
Member
|
@MylesBorins @BridgeAR I’m sorry this bug slipped through – I’ve opened #25577 to address the issue. I’d prefer it if we could fix the bug directly, rather than first reverting the commits in question altogether? |
Sorry, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
20 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
2019-01-17, Version 11.7.0 (Current), @BridgeAR
#22478 is included because of #25446. This should probably not cause any issues due to being on the white list but I would like other collaborators to weight in if we should remove that commit or not (@addaleax you expressed some concerns in the original PR, so please also check again). @nodejs/tsc PTAL.
There is another semver-major commit which is followed by a patch commit that reverts the actual breaking change but that commit is required to allow further backporting.
Notable Changes
Commits