From 6b6474d6c0575bdbcb3363f0544885f7da7ddba1 Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Thu, 26 Oct 2017 16:14:35 -0700 Subject: [PATCH 01/10] doc: add Support section in README Add a Support section, borrowing heavily from wp-cli project. Move stuff about contributing to Node.js to the bottom as vastly more users are interested in using Node.js and getting help with Node.js than contributing to Node.js. Information still belongs, just not at the top. (Many people will know to look in CONTRIBUTING.md anyway.) PR-URL: https://github.com/nodejs/node/pull/16533 Reviewed-By: Vse Mozhet Byt Reviewed-By: Refael Ackermann Reviewed-By: Gireesh Punathil Reviewed-By: Jeremiah Senkpiel Reviewed-By: Joyee Cheung Reviewed-By: James M Snell Reviewed-By: Michael Dawson --- README.md | 47 +++++++++++++++++++++++++++-------------------- 1 file changed, 27 insertions(+), 20 deletions(-) diff --git a/README.md b/README.md index db22c07cd43..286f6f1e8f8 100644 --- a/README.md +++ b/README.md @@ -19,13 +19,10 @@ policies, and releases are managed under an **This project is bound by a [Code of Conduct][].** -If you need help using or installing Node.js, please use the -[nodejs/help](https://github.com/nodejs/help) issue tracker. - # Table of Contents -* [Resources for Newcomers](#resources-for-newcomers) +* [Support](#support) * [Release Types](#release-types) * [Download](#download) * [Current and LTS Releases](#current-and-lts-releases) @@ -39,25 +36,29 @@ If you need help using or installing Node.js, please use the * [Collaborators](#collaborators) * [Release Team](#release-team) -## Resources for Newcomers +## Support + +Node.js contributors have limited availability to address general support +questions. Please make sure you are using a [currently-supported version of +Node.js](https://github.com/nodejs/Release#release-schedule). -### Official Resources +When looking for support, please first search for your question in these venues: -* [Website][] +* [Node.js Website][] * [Node.js Help][] -* [Contributing to the project][] -* IRC (node core development): [#node-dev on chat.freenode.net][] +* [Open or closed issues in the Node.js GitHub organization](https://github.com/issues?utf8=%E2%9C%93&q=sort%3Aupdated-desc+org%3Anodejs+is%3Aissue) +* [Questions tagged 'node.js' on StackOverflow][] + +If you didn't find an answer in one of the venues above, you can: -### Unofficial Resources +* Join the **unofficial** [#node.js channel on chat.freenode.net][]. See + for more information. -* IRC (general questions): [#node.js on chat.freenode.net][]. Please see - for more information regarding the `#node.js` IRC -channel. +GitHub issues are meant for tracking enhancements and bugs, not general support. -_Please note that unofficial resources are neither managed by (nor necessarily -endorsed by) the Node.js TSC. Specifically, such resources are not -currently covered by the [Node.js Moderation Policy][] and the selection and -actions of resource operators/moderators are not subject to TSC oversight._ +Remember, libre != gratis; the open source license grants you the freedom to use +and modify, but not commitments of other people's time. Please be respectful, +and set your expectations accordingly. ## Release Types @@ -582,11 +583,17 @@ Previous releases may also have been signed with one of the following GPG keys: Information on the current Node.js Working Groups can be found in the [TSC repository](https://github.com/nodejs/TSC/blob/master/WORKING_GROUPS.md). +### Contributing to Node.js + +* [Contributing to the project][] +* IRC (node core development): [#node-dev on chat.freenode.net][] + [npm]: https://www.npmjs.com -[Website]: https://nodejs.org/en/ +[Code of Conduct]: https://github.com/nodejs/TSC/blob/master/CODE_OF_CONDUCT.md [Contributing to the project]: CONTRIBUTING.md [Node.js Help]: https://github.com/nodejs/help [Node.js Moderation Policy]: https://github.com/nodejs/TSC/blob/master/Moderation-Policy.md -[#node.js on chat.freenode.net]: https://webchat.freenode.net?channels=node.js&uio=d4 +[Node.js Website]: https://nodejs.org/en/ +[Questions tagged 'node.js' on StackOverflow]: https://stackoverflow.com/questions/tagged/node.js +[#node.js channel on chat.freenode.net]: https://webchat.freenode.net?channels=node.js&uio=d4 [#node-dev on chat.freenode.net]: https://webchat.freenode.net?channels=node-dev&uio=d4 -[Code of Conduct]: https://github.com/nodejs/TSC/blob/master/CODE_OF_CONDUCT.md From ca3f9b75851aea0f82860ee17ddd907b57129121 Mon Sep 17 00:00:00 2001 From: Vse Mozhet Byt Date: Sat, 18 Nov 2017 00:40:26 +0200 Subject: [PATCH 02/10] doc: delete unused definition in README.md PR-URL: https://github.com/nodejs/node/pull/17108 Reviewed-By: Refael Ackermann Reviewed-By: Anna Henningsen --- README.md | 1 - 1 file changed, 1 deletion(-) diff --git a/README.md b/README.md index 286f6f1e8f8..b01308fa7b1 100644 --- a/README.md +++ b/README.md @@ -592,7 +592,6 @@ Information on the current Node.js Working Groups can be found in the [Code of Conduct]: https://github.com/nodejs/TSC/blob/master/CODE_OF_CONDUCT.md [Contributing to the project]: CONTRIBUTING.md [Node.js Help]: https://github.com/nodejs/help -[Node.js Moderation Policy]: https://github.com/nodejs/TSC/blob/master/Moderation-Policy.md [Node.js Website]: https://nodejs.org/en/ [Questions tagged 'node.js' on StackOverflow]: https://stackoverflow.com/questions/tagged/node.js [#node.js channel on chat.freenode.net]: https://webchat.freenode.net?channels=node.js&uio=d4 From ae558af7bc1db217eb1c368c8b5e5773910232c3 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 16 Nov 2017 00:43:12 +0100 Subject: [PATCH 03/10] timers: cross JS/C++ border less frequently This removes the `process._needImmediateCallback` property and its semantics of having a 1/0 switch that tells C++ whether immediates are currently scheduled. Instead, a counter keeping track of all immediates is created, that can be increased on `setImmediate()` or decreased when an immediate is run or cleared. This is faster, because rather than reading/writing a C++ getter, this operation can be performed as a direct memory read/write via a typed array. The only C++ call that is left to make is activating the native handles upon creation of the first `Immediate` after the queue is empty. One other (good!) side-effect is that `immediate._destroyed` now reliably tells whether an `immediate` is still scheduled to run or not. Also, as a nice extra, this should make it easier to implement an internal variant of `setImmediate` for C++ that piggybacks off the same mechanism, which should be useful at least for async hooks and HTTP/2. Benchmark results: $ ./node benchmark/compare.js --new ./node --old ./node-master-1b093cb93df0 --runs 10 --filter immediate timers | Rscript benchmark/compare.R [00:08:53|% 100| 4/4 files | 20/20 runs | 1/1 configs]: Done improvement confidence p.value timers/immediate.js type="breadth" thousands=2000 25.61 % ** 1.432301e-03 timers/immediate.js type="breadth1" thousands=2000 7.66 % 1.320233e-01 timers/immediate.js type="breadth4" thousands=2000 4.61 % 5.669053e-01 timers/immediate.js type="clear" thousands=2000 311.40 % *** 3.896291e-07 timers/immediate.js type="depth" thousands=2000 17.54 % ** 9.755389e-03 timers/immediate.js type="depth1" thousands=2000 17.09 % *** 7.176229e-04 timers/set-immediate-breadth-args.js millions=5 10.63 % * 4.250034e-02 timers/set-immediate-breadth.js millions=10 20.62 % *** 9.150439e-07 timers/set-immediate-depth-args.js millions=10 17.97 % *** 6.819135e-10 PR-URL: https://github.com/nodejs/node/pull/17064 Reviewed-By: Refael Ackermann Reviewed-By: Minwoo Jung Reviewed-By: James M Snell Reviewed-By: Ben Noordhuis --- lib/timers.js | 46 ++++++++++++++------------- src/env-inl.h | 6 ++++ src/env.h | 4 +++ src/node.cc | 86 +++++++++++++++++++++------------------------------ 4 files changed, 69 insertions(+), 73 deletions(-) diff --git a/lib/timers.js b/lib/timers.js index ce27be69450..56f02fe1ea6 100644 --- a/lib/timers.js +++ b/lib/timers.js @@ -47,6 +47,13 @@ const { kInit, kDestroy, kAsyncIdCounter } = async_wrap.constants; const async_id_symbol = Symbol('asyncId'); const trigger_async_id_symbol = Symbol('triggerAsyncId'); +/* This is an Uint32Array for easier sharing with C++ land. */ +const scheduledImmediateCount = process._scheduledImmediateCount; +delete process._scheduledImmediateCount; +/* Kick off setImmediate processing */ +const activateImmediateCheck = process._activateImmediateCheck; +delete process._activateImmediateCheck; + // Timeout values > TIMEOUT_MAX are set to 1. const TIMEOUT_MAX = 2147483647; // 2^31-1 @@ -742,15 +749,9 @@ function processImmediate() { else immediate = next; } - - // Only round-trip to C++ land if we have to. Calling clearImmediate() on an - // immediate that's in |queue| is okay. Worst case is we make a superfluous - // call to NeedImmediateCallbackSetter(). - if (!immediateQueue.head) { - process._needImmediateCallback = false; - } } +process._immediateCallback = processImmediate; // An optimization so that the try/finally only de-optimizes (since at least v8 // 4.7) what is in this smaller function. @@ -762,13 +763,17 @@ function tryOnImmediate(immediate, oldTail) { runCallback(immediate); threw = false; } finally { - // clearImmediate checks _onImmediate === null for kDestroy hooks. immediate._onImmediate = null; if (!threw) emitAfter(immediate[async_id_symbol]); - if (async_hook_fields[kDestroy] > 0 && !immediate._destroyed) { - emitDestroy(immediate[async_id_symbol]); + + if (!immediate._destroyed) { immediate._destroyed = true; + scheduledImmediateCount[0]--; + + if (async_hook_fields[kDestroy] > 0) { + emitDestroy(immediate[async_id_symbol]); + } } if (threw && immediate._idleNext) { @@ -870,10 +875,9 @@ function createImmediate(args, callback) { immediate._argv = args; immediate._onImmediate = callback; - if (!process._needImmediateCallback) { - process._needImmediateCallback = true; - process._immediateCallback = processImmediate; - } + if (scheduledImmediateCount[0] === 0) + activateImmediateCheck(); + scheduledImmediateCount[0]++; immediateQueue.append(immediate); @@ -884,18 +888,16 @@ function createImmediate(args, callback) { exports.clearImmediate = function(immediate) { if (!immediate) return; - if (async_hook_fields[kDestroy] > 0 && - immediate._onImmediate !== null && - !immediate._destroyed) { - emitDestroy(immediate[async_id_symbol]); + if (!immediate._destroyed) { + scheduledImmediateCount[0]--; immediate._destroyed = true; + + if (async_hook_fields[kDestroy] > 0) { + emitDestroy(immediate[async_id_symbol]); + } } immediate._onImmediate = null; immediateQueue.remove(immediate); - - if (!immediateQueue.head) { - process._needImmediateCallback = false; - } }; diff --git a/src/env-inl.h b/src/env-inl.h index 4dadf5ea2c1..f7a7559b3f2 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -283,6 +283,7 @@ inline Environment::Environment(IsolateData* isolate_data, abort_on_uncaught_exception_(false), emit_napi_warning_(true), makecallback_cntr_(0), + scheduled_immediate_count_(isolate_, 1), #if HAVE_INSPECTOR inspector_agent_(new inspector::Agent(this)), #endif @@ -512,6 +513,11 @@ inline void Environment::set_fs_stats_field_array(double* fields) { fs_stats_field_array_ = fields; } +inline AliasedBuffer& +Environment::scheduled_immediate_count() { + return scheduled_immediate_count_; +} + inline performance::performance_state* Environment::performance_state() { return performance_state_; } diff --git a/src/env.h b/src/env.h index 3276a4b9a75..862431df2dd 100644 --- a/src/env.h +++ b/src/env.h @@ -622,6 +622,8 @@ class Environment { inline double* fs_stats_field_array() const; inline void set_fs_stats_field_array(double* fields); + inline AliasedBuffer& scheduled_immediate_count(); + inline performance::performance_state* performance_state(); inline std::map* performance_marks(); @@ -731,6 +733,8 @@ class Environment { size_t makecallback_cntr_; std::vector destroy_async_id_list_; + AliasedBuffer scheduled_immediate_count_; + performance::performance_state* performance_state_ = nullptr; std::map performance_marks_; diff --git a/src/node.cc b/src/node.cc index a1d5ad7e254..81ea92b7e20 100644 --- a/src/node.cc +++ b/src/node.cc @@ -400,25 +400,6 @@ static void PrintErrorString(const char* format, ...) { va_end(ap); } - -static void CheckImmediate(uv_check_t* handle) { - Environment* env = Environment::from_immediate_check_handle(handle); - HandleScope scope(env->isolate()); - Context::Scope context_scope(env->context()); - MakeCallback(env->isolate(), - env->process_object(), - env->immediate_callback_string(), - 0, - nullptr, - {0, 0}).ToLocalChecked(); -} - - -static void IdleImmediateDummy(uv_idle_t* handle) { - // Do nothing. Only for maintaining event loop. - // TODO(bnoordhuis) Maybe make libuv accept nullptr idle callbacks. -} - const char *signo_string(int signo) { #define SIGNO_CASE(e) case e: return #e; switch (signo) { @@ -3019,39 +3000,40 @@ static void DebugEnd(const FunctionCallbackInfo& args); namespace { -void NeedImmediateCallbackGetter(Local property, - const PropertyCallbackInfo& info) { - Environment* env = Environment::GetCurrent(info); - const uv_check_t* immediate_check_handle = env->immediate_check_handle(); - bool active = uv_is_active( - reinterpret_cast(immediate_check_handle)); - info.GetReturnValue().Set(active); +bool MaybeStopImmediate(Environment* env) { + if (env->scheduled_immediate_count()[0] == 0) { + uv_check_stop(env->immediate_check_handle()); + uv_idle_stop(env->immediate_idle_handle()); + return true; + } + return false; } +void CheckImmediate(uv_check_t* handle) { + Environment* env = Environment::from_immediate_check_handle(handle); + HandleScope scope(env->isolate()); + Context::Scope context_scope(env->context()); -void NeedImmediateCallbackSetter( - Local property, - Local value, - const PropertyCallbackInfo& info) { - Environment* env = Environment::GetCurrent(info); + if (MaybeStopImmediate(env)) + return; - uv_check_t* immediate_check_handle = env->immediate_check_handle(); - bool active = uv_is_active( - reinterpret_cast(immediate_check_handle)); + MakeCallback(env->isolate(), + env->process_object(), + env->immediate_callback_string(), + 0, + nullptr, + {0, 0}).ToLocalChecked(); - if (active == value->BooleanValue()) - return; + MaybeStopImmediate(env); +} - uv_idle_t* immediate_idle_handle = env->immediate_idle_handle(); - if (active) { - uv_check_stop(immediate_check_handle); - uv_idle_stop(immediate_idle_handle); - } else { - uv_check_start(immediate_check_handle, CheckImmediate); - // Idle handle is needed only to stop the event loop from blocking in poll. - uv_idle_start(immediate_idle_handle, IdleImmediateDummy); - } +void ActivateImmediateCheck(const FunctionCallbackInfo& args) { + Environment* env = Environment::GetCurrent(args); + uv_check_start(env->immediate_check_handle(), CheckImmediate); + // Idle handle is needed only to stop the event loop from blocking in poll. + uv_idle_start(env->immediate_idle_handle(), + [](uv_idle_t*){ /* do nothing, just keep the loop running */ }); } @@ -3277,12 +3259,11 @@ void SetupProcessObject(Environment* env, FIXED_ONE_BYTE_STRING(env->isolate(), "ppid"), GetParentProcessId).FromJust()); - auto need_immediate_callback_string = - FIXED_ONE_BYTE_STRING(env->isolate(), "_needImmediateCallback"); - CHECK(process->SetAccessor(env->context(), need_immediate_callback_string, - NeedImmediateCallbackGetter, - NeedImmediateCallbackSetter, - env->as_external()).FromJust()); + auto scheduled_immediate_count = + FIXED_ONE_BYTE_STRING(env->isolate(), "_scheduledImmediateCount"); + CHECK(process->Set(env->context(), + scheduled_immediate_count, + env->scheduled_immediate_count().GetJSArray()).FromJust()); // -e, --eval if (eval_string) { @@ -3408,6 +3389,9 @@ void SetupProcessObject(Environment* env, env->as_external()).FromJust()); // define various internal methods + env->SetMethod(process, + "_activateImmediateCheck", + ActivateImmediateCheck); env->SetMethod(process, "_startProfilerIdleNotifier", StartProfilerIdleNotifier); From 02590b4cfa2cefc512cdee4244490a09676a58b2 Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Thu, 2 Nov 2017 22:12:06 -0700 Subject: [PATCH 04/10] tools: fail tests if malformed status file PR-URL: https://github.com/nodejs/node/pull/16703 Reviewed-By: Ben Noordhuis Reviewed-By: Colin Ihrig Reviewed-By: Anatoli Papirovski Reviewed-By: James M Snell Reviewed-By: Gireesh Punathil Reviewed-By: Refael Ackermann Reviewed-By: Luigi Pinca --- tools/test.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/tools/test.py b/tools/test.py index 48d4a1a1eaa..4cb0bd631dc 100755 --- a/tools/test.py +++ b/tools/test.py @@ -1349,9 +1349,7 @@ def ReadConfigurationInto(path, sections, defs): if prefix_match: prefix = SplitPath(prefix_match.group(1).strip()) continue - print "Malformed line: '%s'." % line - return False - return True + raise Exception("Malformed line: '%s'." % line) # --------------- From 780c2d3917ccffd9d71d484b089f74af18355314 Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Sat, 4 Nov 2017 20:49:27 -0700 Subject: [PATCH 05/10] test: move timing-sensitive test to sequential test-https-server-keep-alive-timeout relies on server timeouts and whatnot that will be inherently unreliable on a busy host. The test fails when run with a high `-j` value and higher `--repeat` value passed to `tools/test.py`. Move the test to `sequential`. PR-URL: https://github.com/nodejs/node/pull/16775 Reviewed-By: Colin Ihrig Reviewed-By: Gireesh Punathil Reviewed-By: Khaidi Chu Reviewed-By: James M Snell --- .../test-https-server-keep-alive-timeout.js | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename test/{parallel => sequential}/test-https-server-keep-alive-timeout.js (100%) diff --git a/test/parallel/test-https-server-keep-alive-timeout.js b/test/sequential/test-https-server-keep-alive-timeout.js similarity index 100% rename from test/parallel/test-https-server-keep-alive-timeout.js rename to test/sequential/test-https-server-keep-alive-timeout.js From cfda245706249255b517c2c76252c50377f7c616 Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Sun, 5 Nov 2017 02:12:23 -0800 Subject: [PATCH 06/10] benchmark: use unique filenames in fs benchmarks Use a unique file name for each benchmark. Running benchmarks simultaneously may be a bit of an unusual use case, but there are use cases, such as stress testing `test/parallel/test-benchmark-fs.js`. PR-URL: https://github.com/nodejs/node/pull/16776 Reviewed-By: Colin Ihrig Reviewed-By: Gireesh Punathil Reviewed-By: Luigi Pinca Reviewed-By: James M Snell --- benchmark/fs/read-stream-throughput.js | 3 ++- benchmark/fs/readfile.js | 3 ++- benchmark/fs/write-stream-throughput.js | 3 ++- 3 files changed, 6 insertions(+), 3 deletions(-) diff --git a/benchmark/fs/read-stream-throughput.js b/benchmark/fs/read-stream-throughput.js index 4412d41f98d..85aa3c929fe 100644 --- a/benchmark/fs/read-stream-throughput.js +++ b/benchmark/fs/read-stream-throughput.js @@ -3,7 +3,8 @@ const path = require('path'); const common = require('../common.js'); -const filename = path.resolve(__dirname, '.removeme-benchmark-garbage'); +const filename = path.resolve(__dirname, + `.removeme-benchmark-garbage-${process.pid}`); const fs = require('fs'); const assert = require('assert'); diff --git a/benchmark/fs/readfile.js b/benchmark/fs/readfile.js index d89061d6061..82479ff14e5 100644 --- a/benchmark/fs/readfile.js +++ b/benchmark/fs/readfile.js @@ -5,7 +5,8 @@ const path = require('path'); const common = require('../common.js'); -const filename = path.resolve(__dirname, '.removeme-benchmark-garbage'); +const filename = path.resolve(__dirname, + `.removeme-benchmark-garbage-${process.pid}`); const fs = require('fs'); const bench = common.createBenchmark(main, { diff --git a/benchmark/fs/write-stream-throughput.js b/benchmark/fs/write-stream-throughput.js index 2083cccbd6a..5c6464fdbb2 100644 --- a/benchmark/fs/write-stream-throughput.js +++ b/benchmark/fs/write-stream-throughput.js @@ -3,7 +3,8 @@ const path = require('path'); const common = require('../common.js'); -const filename = path.resolve(__dirname, '.removeme-benchmark-garbage'); +const filename = path.resolve(__dirname, + `.removeme-benchmark-garbage-${process.pid}`); const fs = require('fs'); const bench = common.createBenchmark(main, { From 0a1fba02a65b0b6bb53ca2d6966eaca67265e7d6 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Thu, 16 Nov 2017 03:10:18 +0800 Subject: [PATCH 07/10] doc: reorganize collaborator guide MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Add sections about first time contributions, code reviews and seeking consensus, waiting for approvals, testing and CI * Move paragraphs to more suitable sections * Update table of contents * Document the fast-tracking process PR-URL: https://github.com/nodejs/node/pull/17056 Reviewed-By: James M Snell Reviewed-By: Tobias Nießen Reviewed-By: Michael Dawson Reviewed-By: Gireesh Punathil --- COLLABORATOR_GUIDE.md | 190 +++++++++++++++++++++++++++--------------- 1 file changed, 122 insertions(+), 68 deletions(-) diff --git a/COLLABORATOR_GUIDE.md b/COLLABORATOR_GUIDE.md index c61aad6e7d4..d8d0651ff53 100644 --- a/COLLABORATOR_GUIDE.md +++ b/COLLABORATOR_GUIDE.md @@ -3,16 +3,34 @@ **Contents** * [Issues and Pull Requests](#issues-and-pull-requests) + - [Managing Issues and Pull Requests](#managing-issues-and-pull-requests) + - [Welcoming First-Time Contributiors](#welcoming-first-time-contributiors) + - [Closing Issues and Pull Requests](#closing-issues-and-pull-requests) * [Accepting Modifications](#accepting-modifications) - - [Useful CI Jobs](#useful-ci-jobs) - - [Internal vs. Public API](#internal-vs-public-api) - - [Breaking Changes](#breaking-changes) - - [Deprecations](#deprecations) - - [Involving the TSC](#involving-the-tsc) + - [Code Reviews and Consensus Seeking](#code-reviews-and-consensus-seeking) + - [Waiting for Approvals](#waiting-for-approvals) + - [Testing and CI](#testing-and-ci) + - [Useful CI Jobs](#useful-ci-jobs) + - [Internal vs. Public API](#internal-vs-public-api) + - [Breaking Changes](#breaking-changes) + - [Breaking Changes and Deprecations](#breaking-changes-and-deprecations) + - [Breaking Changes to Internal Elements](#breaking-changes-to-internal-elements) + - [When Breaking Changes Actually Break Things](#when-breaking-changes-actually-break-things) + - [Reverting commits](#reverting-commits) + - [Introducing New Modules](#introducing-new-modules) + - [Deprecations](#deprecations) + - [Involving the TSC](#involving-the-tsc) * [Landing Pull Requests](#landing-pull-requests) - - [Technical HOWTO](#technical-howto) - - [I Just Made a Mistake](#i-just-made-a-mistake) - - [Long Term Support](#long-term-support) + - [Technical HOWTO](#technical-howto) + - [Troubleshooting](#troubleshooting) + - [I Just Made a Mistake](#i-just-made-a-mistake) + - [Long Term Support](#long-term-support) + - [What is LTS?](#what-is-lts) + - [How does LTS work?](#how-does-lts-work) + - [Landing semver-minor commits in LTS](#landing-semver-minor-commits-in-lts) + - [How are LTS Branches Managed?](#how-are-lts-branches-managed) + - [How can I help?](#how-can-i-help) + - [How is an LTS release cut?](#how-is-an-lts-release-cut) This document contains information for Collaborators of the Node.js project regarding maintaining the code, documentation and issues. @@ -24,16 +42,31 @@ understand the project governance model as outlined in ## Issues and Pull Requests -Courtesy should **always** be shown to individuals submitting issues and pull -requests to the Node.js project. Be welcoming to first time contributors, -identified by the GitHub ![badge](./doc/first_timer_badge.png) badge. +### Managing Issues and Pull Requests Collaborators should feel free to take full responsibility for managing issues and pull requests they feel qualified to handle, as long as this is done while being mindful of these guidelines, the -opinions of other Collaborators and guidance of the TSC. +opinions of other Collaborators and guidance of the [TSC][]. They +may also notify other qualified parties for more input on an issue +or a pull request. +[See "Who to CC in issues"](./doc/onboarding-extras.md#who-to-cc-in-issues) + +### Welcoming First-Time Contributiors -Collaborators may **close** any issue or pull request they believe is +Courtesy should always be shown to individuals submitting issues and pull +requests to the Node.js project. Be welcoming to first-time contributors, +identified by the GitHub ![badge](./doc/first_timer_badge.png) badge. + +For first-time contributors, check if the commit author is the same as the +pull request author, and ask if they have configured their git +username and email to their liking as per [this guide][git-username]. +This is to make sure they would be promoted to "contributor" once +their pull request gets landed. + +### Closing Issues and Pull Requests + +Collaborators may close any issue or pull request they believe is not relevant for the future of the Node.js project. Where this is unclear, the issue should be left open for several days to allow for additional discussion. Where this does not yield input from Node.js @@ -41,13 +74,14 @@ Collaborators or additional evidence that the issue has relevance, the issue may be closed. Remember that issues can always be re-opened if necessary. -[**See "Who to CC in issues"**](./doc/onboarding-extras.md#who-to-cc-in-issues) - ## Accepting Modifications All modifications to the Node.js code and documentation should be performed via GitHub pull requests, including modifications by -Collaborators and TSC members. +Collaborators and TSC members. A pull request must be reviewed, and usually +must also be tested with CI, before being landed into the codebase. + +### Code Reviews and Consensus Seeking All pull requests must be reviewed and accepted by a Collaborator with sufficient expertise who is able to take full responsibility for the @@ -55,22 +89,17 @@ change. In the case of pull requests proposed by an existing Collaborator, an additional Collaborator is required for sign-off. In some cases, it may be necessary to summon a qualified Collaborator -to a pull request for review by @-mention. +or a Github team to a pull request for review by @-mention. +[See "Who to CC in issues"](./doc/onboarding-extras.md#who-to-cc-in-issues) If you are unsure about the modification and are not prepared to take full responsibility for the change, defer to another Collaborator. -Before landing pull requests, sufficient time should be left for input -from other Collaborators. Leave at least 48 hours during the week and -72 hours over weekends to account for international time differences -and work schedules. Trivial changes (e.g. those which fix minor bugs -or improve performance without affecting API or causing other -wide-reaching impact), and focused changes that affect only documentation -and/or the test suite, may be landed after a shorter delay if they have -multiple approvals. - -For first time contributors, ask the author if they have configured their git -username and email to their liking as per [this guide][git-username]. +If any Collaborator objects to a change *without giving any additional +explanation or context*, and the objecting Collaborator fails to respond to +explicit requests for explanation or context within a reasonable period of +time, the objection may be dismissed. Note that this does not apply to +objections that are explained. For non-breaking changes, if there is no disagreement amongst Collaborators, a pull request may be landed given appropriate review. @@ -80,12 +109,32 @@ elevate discussion to the TSC for resolution (see below). Breaking changes (that is, pull requests that require an increase in the major version number, known as `semver-major` changes) must be -elevated for review by the TSC. This does not necessarily mean that the -PR must be put onto the TSC meeting agenda. If multiple TSC members -approve (`LGTM`) the PR and no Collaborators oppose the PR, it can be -landed. Where there is disagreement among TSC members or objections -from one or more Collaborators, `semver-major` pull requests should be -put on the TSC meeting agenda. +[elevated for review by the TSC](#involving-the-tsc). +This does not necessarily mean that the PR must be put onto the TSC meeting +agenda. If multiple TSC members approve (`LGTM`) the PR and no Collaborators +oppose the PR, it can be landed. Where there is disagreement among TSC members +or objections from one or more Collaborators, `semver-major` pull requests +should be put on the TSC meeting agenda. + +### Waiting for Approvals + +Before landing pull requests, sufficient time should be left for input +from other Collaborators. In general, leave at least 48 hours during the +week and 72 hours over weekends to account for international time +differences and work schedules. However, certain types of pull requests +can be fast-tracked and may be landed after a shorter delay: + +* Focused changes that affect only documentation and/or the test suite. + `code-and-learn` and `good-first-issue` pull requests typically fall + into this category. +* Changes that revert commit(s) and/or fix regressions. + +When a pull request is deemed suitable to be fast-tracked, label it with +`fast-track`. The pull request can be landed once 2 or more collaborators +approve both the pull request and the fast-tracking request, and the necessary +CI testing is done. + +### Testing and CI All bugfixes require a test case which demonstrates the defect. The test should *fail* before the change, and *pass* after the change. @@ -94,12 +143,6 @@ All pull requests that modify executable code should be subjected to continuous integration tests on the [project CI server](https://ci.nodejs.org/). -If any Collaborator objects to a change *without giving any additional -explanation or context*, and the objecting Collaborator fails to respond to -explicit requests for explanation or context within a reasonable period of -time, the objection may be dismissed. Note that this does not apply to -objections that are explained. - #### Useful CI Jobs * [`node-test-pull-request`](https://ci.nodejs.org/job/node-test-pull-request/) @@ -172,20 +215,8 @@ using an API in a manner currently undocumented achieves a particular useful result, a decision will need to be made whether or not that falls within the supported scope of that API; and if it does, it should be documented. -Breaking changes to internal elements are permitted in semver-patch or -semver-minor commits but Collaborators should take significant care when -making and reviewing such changes. Before landing such commits, an effort -must be made to determine the potential impact of the change in the ecosystem -by analyzing current use and by validating such changes through ecosystem -testing using the [Canary in the Goldmine](https://github.com/nodejs/citgm) -tool. If a change cannot be made without ecosystem breakage, then TSC review is -required before landing the change as anything less than semver-major. - -If a determination is made that a particular internal API (for instance, an -underscore `_` prefixed property) is sufficiently relied upon by the ecosystem -such that any changes may break user code, then serious consideration should be -given to providing an alternative Public API for that functionality before any -breaking changes are made. +See [Breaking Changes to Internal Elements](#breaking-changes-to-internal-elements) +on how to handle those types of changes. ### Breaking Changes @@ -200,6 +231,13 @@ changing error messages in any way, altering expected timing of an event (e.g. moving from sync to async responses or vice versa), and changing the non-internal side effects of using a particular API. +Purely additive changes (e.g. adding new events to `EventEmitter` +implementations, adding new arguments to a method in a way that allows +existing code to continue working without modification, or adding new +properties to an options argument) are semver-minor changes. + +#### Breaking Changes and Deprecations + With a few notable exceptions outlined below, when backwards incompatible changes to a *Public* API are necessary, the existing API *must* be deprecated *first* and the new API either introduced in parallel or added after the next @@ -216,14 +254,6 @@ Exception to this rule is given in the following cases: Such changes *must* be handled as semver-major changes but MAY be landed without a [Deprecation cycle](#deprecation-cycle). -From time-to-time, in particularly exceptional cases, the TSC may be asked to -consider and approve additional exceptions to this rule. - -Purely additive changes (e.g. adding new events to EventEmitter -implementations, adding new arguments to a method in a way that allows -existing code to continue working without modification, or adding new -properties to an options argument) are handled as semver-minor changes. - Note that errors thrown, along with behaviors and APIs implemented by dependencies of Node.js (e.g. those originating from V8) are generally not under the control of Node.js and therefore *are not directly subject to this @@ -231,7 +261,29 @@ policy*. However, care should still be taken when landing updates to dependencies when it is known or expected that breaking changes to error handling may have been made. Additional CI testing may be required. -#### When breaking changes actually break things +From time-to-time, in particularly exceptional cases, the TSC may be asked to +consider and approve additional exceptions to this rule. + +For more information, see [Deprecations](#deprecations). + +#### Breaking Changes to Internal Elements + +Breaking changes to internal elements are permitted in semver-patch or +semver-minor commits but Collaborators should take significant care when +making and reviewing such changes. Before landing such commits, an effort +must be made to determine the potential impact of the change in the ecosystem +by analyzing current use and by validating such changes through ecosystem +testing using the [Canary in the Goldmine](https://github.com/nodejs/citgm) +tool. If a change cannot be made without ecosystem breakage, then TSC review is +required before landing the change as anything less than semver-major. + +If a determination is made that a particular internal API (for instance, an +underscore `_` prefixed property) is sufficiently relied upon by the ecosystem +such that any changes may break user code, then serious consideration should be +given to providing an alternative Public API for that functionality before any +breaking changes are made. + +#### When Breaking Changes Actually Break Things Because breaking (semver-major) changes are permitted to land on the master branch at any time, at least some subset of the user ecosystem may be adversely @@ -349,12 +401,13 @@ Changes" section of the release notes. ### Involving the TSC -Collaborators may opt to elevate pull requests or issues to the TSC for -discussion by assigning the `tsc-review` label. This should be done -where a pull request: +Collaborators may opt to elevate pull requests or issues to the [TSC][] for +discussion by assigning the `tsc-review` label or @-mentioning the +`@nodejs/tsc` Github team. This should be done where a pull request: -- has a significant impact on the codebase, -- is inherently controversial; or +- is labeled `semver-major`, or +- has a significant impact on the codebase, or +- is inherently controversial, or - has failed to reach consensus amongst the Collaborators who are actively participating in the discussion. @@ -681,3 +734,4 @@ LTS working group and the Release team. [Enhancement Proposal]: https://github.com/nodejs/node-eps [git-username]: https://help.github.com/articles/setting-your-username-in-git/ [`node-core-utils`]: https://github.com/nodejs/node-core-utils +[TSC]: https://github.com/nodejs/TSC From 45e6642476389fe5442c23b08a3ea99fcc752e56 Mon Sep 17 00:00:00 2001 From: Benjamin Zaslavsky Date: Tue, 14 Nov 2017 23:59:57 +0100 Subject: [PATCH 08/10] console: add support for console.debug MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds the console.debug() method, alias for console.log(). This method is exposed by V8 and was only available in inspector until now. Also adds matching test and documentation. PR-URL: https://github.com/nodejs/node/pull/17033 Refs: https://github.com/nodejs/node/pull/17004 Reviewed-By: Colin Ihrig Reviewed-By: Khaidi Chu Reviewed-By: Michaël Zasso Reviewed-By: Anna Henningsen Reviewed-By: Timothy Gu Reviewed-By: James M Snell --- doc/api/console.md | 9 +++++++++ lib/console.js | 3 +++ test/parallel/test-console.js | 11 +++++++++++ 3 files changed, 23 insertions(+) diff --git a/doc/api/console.md b/doc/api/console.md index 8a9ca1ac861..54d86cea161 100644 --- a/doc/api/console.md +++ b/doc/api/console.md @@ -238,6 +238,15 @@ undefined > ``` +### console.debug(data[, ...args]) + +* `data` {any} +* `...args` {any} + +The `console.debug()` function is an alias for [`console.log()`][]. + ### console.dir(obj[, options])