GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||
GeezerVison is showing github.com — v11.7.0 proposal by BridgeAR · Pull Request #25537 · nodejs/node Conversation
Member
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
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
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 <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Denys Otrishko <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> PR-URL: nodejs#25216 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> 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 <[email protected]> '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 <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> 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 <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Anto Aravinth <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> 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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Khaidi Chu <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> This would otherwise be reported as a memory leak by automated tools. PR-URL: nodejs#25180 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> 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 <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]> 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 <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anto Aravinth <[email protected]> PR-URL: nodejs#25207 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> This commit adds a DCHECK macro for consistency with the other DCHECK_* macros. PR-URL: nodejs#25207 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> 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 <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]> 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 <[email protected]> Reviewed-By: James M Snell <[email protected]> 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 <[email protected]> Reviewed-By: James M Snell <[email protected]> PR-URL: nodejs#25149 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Denys Otrishko <[email protected]> Simplify code by using return value of Object.defineProperty directly. PR-URL: nodejs#25162 Reviewed-By: Vladimir de Turckheim <[email protected]> Reviewed-By: Masashi Hirano <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> 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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> PR-URL: nodejs#25174 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> 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 <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> 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 <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> 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 <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> "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 <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> 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 <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> 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 <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> PR-URL: nodejs#25155 Reviewed-By: James M Snell <[email protected]> PR-URL: nodejs#25155 Reviewed-By: James M Snell <[email protected]> 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 <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Member
Author
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page.
Member
Author
All reactions
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page.
Member
Author
All reactions
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page.
BridgeAR
force-pushed
the
v11.7.0-proposal
branch
from
03f3361 to
b408aa5
Compare
|
Uh oh!
There was an error while loading. Please reload this page.
I think that was before our whitelisting approach for Fwiw, I’d also consider #25361 to be a notable change. |
All reactions
-
👍 2 reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
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. |
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
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
January 17, 2019 19:51
b408aa5 to
6b7b827
Compare
Member
Author
|
I added #25361 as notable change. |
All reactions
-
❤️ 2 reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
PR-URL: nodejs#25537
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. |
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Member
|
Can we at least remove the |
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Contributor
|
Was just reviewing CITGM results and it seems like the node-report failure is legitimate. |
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Contributor
|
it's --> #25258 specifically d718625 Can confirm that reverting the above commit fixes node-reports's test suite /cc @gabrielf @addaleax @rnchamberlain @richardlau |
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Member
|
@MylesBorins is it only in |
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
MylesBorins
commented
Jan 18, 2019
•
edited
Loading
Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
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. edit: here is the SetEvents method https://github.com/nodejs/node-report/blob/master/src/module.cc#L103-L134 |
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Member
|
It would be great if #25562 could make it into the release. |
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
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
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. |
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
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
Contributor
|
Please remove both commits from the offending pr |
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
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? |
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Reviewers
No reviewsUh oh!
There was an error while loading. Please reload this page.
Assignees
No one assignedLabels
brotli
Issues and PRs related to the Brotli dependency.
build
Issues and PRs related to Node.js builds or CI infrastructure.
doc
Issues and PRs related to Node.js documentation.
meta
Issues and PRs related to the general management of the project.
tools
Issues and PRs related to the tools directory.
Projects
Milestone
No milestoneDevelopment
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
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.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.