Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

inspector,cli: restore --debug and --inspect --debug-brk with deprecation warnings #12949

Merged
merged 0 commits into from
May 29, 2017

Conversation

refack
Copy link
Contributor

@refack refack commented May 10, 2017

spinning-off #12580

status summary

  • current generation of IDEs work with both protocols (using version detection)
  • vendors have been exclusively using the --inspect --debug-brk combo to trigger inspector
  • if we don't restore this alias current IDEs will not work with node v8
  • be "noisy" about this being deprecated, and give vendors a year to adjust

motivation

--debug-brk & --debug don't exist after #12197, leaving a current generation of tools without
a way to start node with inspector activated and breaking on first line.

Add --debug-brk and --debug back in as an undocumented and deprecated option until 7.x is no longer
supported.

Fixes: #12364
Fixes: #12630

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

test,inspector,debugger,cli

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. debugger labels May 10, 2017
@refack refack changed the title New 12580 restore debug brk inspector,cli: restore --debug-brk as alias for --inspect-brk May 10, 2017
@refack
Copy link
Contributor Author

refack commented May 10, 2017

Current consensus #12630 (comment):

  • We need to keep --inspect --debug-brk working for the time being.
  • For better usability & less confusion --inspect-brk is still a good thing

if (option_name == "--inspect") {
enable_inspector = true;
} else if (option_name == "--inspect-brk") {
} else if (option_name == "--inspect-brk" || option_name == "--debug-brk") {
Copy link
Member

Choose a reason for hiding this comment

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

This would also require a deprecation warning to be emitted at startup when --debug-brk is used. The CTC discussion also centered on adding --debug back as an alias for --inspect, also with a deprecation warning.

Note: The deprecation warning would need to use the process.emitWarning() mechanism. I believe there is a deprecation code already allocated for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jasnell added that to first comment
I believe that @jkrems also requested the --debug-brk alone will fail, and that only the combo --inspect --debug-brk will be valid.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is how I made it fail: jkrems@50ecfe4

E.g. track --debug-brk being used and then fail (hard) if it was used w/o --inspect. Because that case would mean someone relatively clearly was trying to start the old protocol.

@refack refack added cli Issues and PRs related to the Node.js command line interface. inspector Issues and PRs related to the V8 inspector protocol labels May 10, 2017
@refack refack changed the title inspector,cli: restore --debug-brk as alias for --inspect-brk inspector,cli: restore --debug* as alias for --inspect* May 10, 2017
@refack
Copy link
Contributor Author

refack commented May 10, 2017

Testing the waters CI:https://ci.nodejs.org/job/node-test-commit/9789/

src/node.cc Outdated
@@ -3955,6 +3955,12 @@ static void ParseArgs(int* argc,
exit(9);
}

if (debug_options.debug_brk_used() && !debug_options.inspector_enabled()) {
fprintf(stderr,
"%s: Using --debug-brk without --inspect is no longer supported\n", argv[0]);
Copy link
Member

Choose a reason for hiding this comment

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

This should use the process.emitWarning() mechanism instead. This can be done by setting a flag and later calling process.emitWarning() during bootstrapping.

Copy link
Member

Choose a reason for hiding this comment

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

It should also be a proper deprecation warning... e.g.

process.emitWarning(message, 'DeprecationWarning', 'DEP00NN');

Where DEP00NN is an appropriate deprecation code (see docs/api/deprecations.md)

#endif // HAVE_INSPECTOR
bool wait_connect_;
bool wait_connect_; // --inspect-brk
Copy link
Contributor

Choose a reason for hiding this comment

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

If a comment is helpful here I think it could be a bit clearer - wait_connect_ means to break before executing user code (i.e. on first line) and is used with both --debug-brk and --inspect-brk.

@joshgav
Copy link
Contributor

joshgav commented May 10, 2017

I think we should only support --debug-brk with --inspect, not --debug, since that's all that has been requested. The code here does just that, but the original post and title imply that it adds back --debug too, so wanted to be explicit.

I think it would be helpful to print a message and exit on --debug instead of crashing, but that should probably be in a separate PR.

@refack
Copy link
Contributor Author

refack commented May 10, 2017

I think we should only support --debug-brk with --inspect, not --debug, since that's all that has been requested. The code here does just that, but the original post and title imply that it adds back --debug too, so wanted to be explicit.

I'll phrase a more generic title, since we want --debug[=port] to be a deprecated aliased to --inspect[=port] #12949 (comment) but keep the single --debug-brk[=port] invalid...

I think it would be helpful to print a message and exit on --debug instead of crashing, but that should probably be in a separate PR.

I'll clean that up, since it should be process.emitWarning(message, 'DeprecationWarning', 'DEP00NN'); anyway.

@refack refack changed the title inspector,cli: restore --debug* as alias for --inspect* inspector,cli: restore --debug and --inspect --debug-brk with deprecation warnings May 10, 2017
@joshgav
Copy link
Contributor

joshgav commented May 10, 2017

@refack

we want --debug[=port] to be a deprecated aliased to --inspect[=port]

I'm okay with this if downstream users (or IDEs) need it - do they? As @ChALkeR noted, the behavior of starting the Inspector agent rather than the legacy debugger agent would still be unexpected.

If downstream users don't need it, I think it would be best to print a deprecation message (e.g. "please use --inspect") and exit.

@refack refack added wip Issues and PRs that are still a work in progress. v8.x labels May 10, 2017
@refack
Copy link
Contributor Author

refack commented May 10, 2017

@jasnell re: #12949 (comment) If I understand you correctly, the wanted result is:

  1. --debug == --inspect + deprecation warning
  2. --inspect --debug-brk = --inspect-brk + deprecation warning (for IDEs backward compatibility)
  3. --debug-brk = invalid

@refack
Copy link
Contributor Author

refack commented May 10, 2017

@joshgav I don't know of 3rd party requirements for the above (1) either. But @jasnell mentioned it explicitly.

@jkrems
Copy link
Contributor

jkrems commented May 10, 2017

I don't think (1) is a requirement and I'd argue it would be a dangerous change. If I read it correctly, this PR only restores --inspect --debug-brk and that's the only thing that I heard IDEs asking for. --debug is still an invalid flag after this change.

The CTC notes seem to focus on --debug-brk only:

Next steps

  • Никита to review use as alias for potential problems.
  • If/when Никита approves: restore --debug-brk as an alias to --inspect-brk that also prints a deprecation warning.

@refack
Copy link
Contributor Author

refack commented May 15, 2017

It should also be a proper deprecation warning... e.g.

It's CC side, we don't have a well guarded process.emitWarning.
What would you suggest:

  1. Implement (with test all the noDeprecation throwDeprecation traceDeprecation)
  2. Leak a hint so bootstrap will handle this?

@roblourens
Copy link

Sorry for the spam, but this will definitely still make it into Node 8, right? It seems like the plan is still to ship next week?

@jkrems
Copy link
Contributor

jkrems commented May 26, 2017

It's CC side, we don't have a well guarded process.emitWarning.
What would you suggest: [...]

I think @jasnell was suggesting the bootstrap hint:

This can be done by setting a flag and later calling process.emitWarning() during bootstrapping.

@roblourens Thanks for spamming this PR. Yes, this definitely should get into Node 8. It's not semver-major so we can still land it in node 8 even it isn't part of the very first release. But obviously it would be great if we could get it into 8.0.0 to reduce frustration.

@roblourens
Copy link

After the really extensive discussion we had about this, my understanding was that we could fix it for 8.0.0 so that there wouldn't be any version of Node that vscode and other clients can't debug. Even if we don't have the perfect deprecation warning, I think it's really important to get something out to not leave a gap like that.

@refack
Copy link
Contributor Author

refack commented May 26, 2017

After the really extensive discussion we had about this, my understanding was that we could fix it for 8.0.0 so that there wouldn't be any version of Node that vscode and other clients can't debug. Even if we don't have the perfect deprecation warning, I think it's really important to get something out to not leave a gap like that.

Yep. I'll finish this today.
@roblourens thanks you the ping

jasnell pushed a commit that referenced this pull request May 29, 2017
PR-URL: #12949
Fixes: #12364
Fixes: #12630
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
jasnell added a commit to jasnell/node that referenced this pull request May 29, 2017
* **Async Hooks**
  * The `async_hooks` module has landed in core
    [[`4a7233c178`](nodejs@4a7233c178)]
    [nodejs#12892](nodejs#12892).

* **Buffer**
  * Using the `--pending-deprecation` flag will cause Node.js to emit a
    deprecation warning when using `new Buffer(num)` or `Buffer(num)`.
    [[`d2d32ea5a2`](nodejs@d2d32ea5a2)]
    [nodejs#11968](nodejs#11968).
  * `new Buffer(num)` and `Buffer(num)` will zero-fill new `Buffer` instances
    [[`7eb1b4658e`](nodejs@7eb1b4658e)]
    [nodejs#12141](nodejs#12141).
  * Many `Buffer` methods now accept `Uint8Array` as input
    [[`beca3244e2`](nodejs@beca3244e2)]
    [nodejs#10236](nodejs#10236).

* **Child Process**
  * Argument and kill signal validations have been improved
    [[`97a77288ce`](nodejs@97a77288ce)]
    [nodejs#12348](nodejs#12348),
    [[`d75fdd96aa`](nodejs@d75fdd96aa)]
    [nodejs#10423](nodejs#10423).
  * Child Process methods accept `Uint8Array` as input
    [[`627ecee9ed`](nodejs@627ecee9ed)]
    [nodejs#10653](nodejs#10653).

* **Console**
  * Error events emitted when using `console` methods are now supressed.
    [[`f18e08d820`](nodejs@f18e08d820)]
    [nodejs#9744](nodejs#9744).

* **Dependencies**
  * The npm client has been updated to 5.0.0
    [[`3c3b36af0f`](nodejs@3c3b36af0f)]
    [nodejs#12936](nodejs#12936).
  * V8 has been updated to 5.8 with forward ABI stability to 6.0
    [[`60d1aac8d2`](nodejs@60d1aac8d2)]
    [nodejs#12784](nodejs#12784).

* **Domains**
  * Native `Promise` instances are now `Domain` aware
    [[`84dabe8373`](nodejs@84dabe8373)]
    [nodejs#12489](nodejs#12489).

* **Errors**
  * We have started assigning static error codes to errors generated by Node.js.
    This has been done through multiple commits and is still a work in
    progress.

* **File System**
  * The utility class `fs.SyncWriteStream` has been deprecated
    [[`7a55e34ef4`](nodejs@7a55e34ef4)]
    [nodejs#10467](nodejs#10467).
  * The deprecated `fs.read()` string interface has been removed
    [[`3c2a9361ff`](nodejs@3c2a9361ff)]
    [nodejs#9683](nodejs#9683).

* **HTTP**
  * Improved support for userland implemented Agents
    [[`90403dd1d0`](nodejs@90403dd1d0)]
    [nodejs#11567](nodejs#11567).
  * Outgoing Cookie headers are concatenated into a single string
    [[`d3480776c7`](nodejs@d3480776c7)]
    [nodejs#11259](nodejs#11259).
  * The `httpResponse.writeHeader()` method has been deprecated
    [[`fb71ba4921`](nodejs@fb71ba4921)]
    [nodejs#11355](nodejs#11355).
  * New methods for accessing HTTP headers have been added to `OutgoingMessage`
    [[`3e6f1032a4`](nodejs@3e6f1032a4)]
    [nodejs#10805](nodejs#10805).

* **Lib**
  * All deprecation messages have been assigned static identifiers
    [[`5de3cf099c`](nodejs@5de3cf099c)]
    [nodejs#10116](nodejs#10116).
  * The legacy `linkedlist` module has been removed
    [[`84a23391f6`](nodejs@84a23391f6)]
    [nodejs#12113](nodejs#12113).

* **N-API**
  * Experimental support for the new N-API API has been added
    [[`56e881d0b0`](nodejs@56e881d0b0)]
    [nodejs#11975](nodejs#11975).

* **Process**
  * Process warning output can be redirected to a file using the
    `--redirect-warnings` command-line argument
    [[`03e89b3ff2`](nodejs@03e89b3ff2)]
    [nodejs#10116](nodejs#10116).
  * Process warnings may now include additional detail
    [[`dd20e68b0f`](nodejs@dd20e68b0f)]
    [nodejs#12725](nodejs#12725).

* **REPL**
  * REPL magic mode has been deprecated
    [[`3f27f02da0`](nodejs@3f27f02da0)]
    [nodejs#11599](nodejs#11599).

* **Src**
  * `NODE_MODULE_VERSION` has been updated to 57
    (nodejs@ec7cbaf266)]
    [nodejs#12995](nodejs#12995).
  * Add `--pending-deprecation` command-line argument and
    `NODE_PENDING_DEPRECATION` environment variable
    [[`a16b570f8c`](nodejs@a16b570f8c)]
    [nodejs#11968](nodejs#11968).
  * The `--debug` command-line argument has been deprecated. Note that
    using `--debug` will enable the *new* Inspector-based debug protocol
    as the legacy Debugger protocol previously used by Node.js has been
    removed. [[`010f864426`](nodejs@010f864426)]
    [nodejs#12949](nodejs#12949).
  * Throw when the `-c` and `-e` command-line arguments are used at the same
    time [[`a5f91ab230`](nodejs@a5f91ab230)]
    [nodejs#11689](nodejs#11689).
  * Throw when the `--use-bundled-ca` and `--use-openssl-ca` command-line
    arguments are used at the same time.
    [[`8a7db9d4b5`](nodejs@8a7db9d4b5)]
    [nodejs#12087](nodejs#12087).

* **Stream**
  * `Stream` now supports `destroy()` and `_destroy()` APIs
    [[`b6e1d22fa6`](nodejs@b6e1d22fa6)]
    [nodejs#12925](nodejs#12925).
  * `Stream` now supports the `_final()` API
    [[`07c7f198db`](nodejs@07c7f198db)]
    [nodejs#12828](nodejs#12828).

* **TLS**
  * The `rejectUnauthorized` option now defaults to `true`
    [[`348cc80a3c`](nodejs@348cc80a3c)]
    [nodejs#5923](nodejs#5923).
  * The `tls.createSecurePair()` API now emits a runtime deprecation
    [[`a2ae08999b`](nodejs@a2ae08999b)]
    [nodejs#11349](nodejs#11349).
  * A runtime deprecation will now be emitted when `dhparam` is less than
    2048 bits [[`d523eb9c40`](nodejs@d523eb9c40)]
    [nodejs#11447](nodejs#11447).

* **URL**
  * The WHATWG URL implementation is now a fully-supported Node.js API
    [[`d080ead0f9`](nodejs@d080ead0f9)]
    [nodejs#12710](nodejs#12710).

* **Util**
  * `Symbol` keys are now displayed by default when using `util.inspect()`
    [[`5bfd13b81e`](nodejs@5bfd13b81e)]
    [nodejs#9726](nodejs#9726).
  * `toJSON` errors will be thrown when formatting `%j`
    [[`455e6f1dd8`](nodejs@455e6f1dd8)]
    [nodejs#11708](nodejs#11708).
  * Convert `inspect.styles` and `inspect.colors` to prototype-less objects
    [[`aab0d202f8`](nodejs@aab0d202f8)]
    [nodejs#11624](nodejs#11624).
  * The new `util.promisify()` API has been added
    [[`99da8e8e02`](nodejs@99da8e8e02)]
    [nodejs#12442](nodejs#12442).

* **Zlib**
  * Support `Uint8Array` in Zlib convenience methods
    [[`91383e47fd`](nodejs@91383e47fd)]
    [nodejs#12001](nodejs#12001).
  * Zlib errors now use `RangeError` and `TypeError` consistently
    [[`b514bd231e`](nodejs@b514bd231e)]
    [nodejs#11391](nodejs#11391).
jasnell added a commit that referenced this pull request May 30, 2017
* **Async Hooks**
  * The `async_hooks` module has landed in core
    [[`4a7233c178`](4a7233c178)]
    [#12892](#12892).

* **Buffer**
  * Using the `--pending-deprecation` flag will cause Node.js to emit a
    deprecation warning when using `new Buffer(num)` or `Buffer(num)`.
    [[`d2d32ea5a2`](d2d32ea5a2)]
    [#11968](#11968).
  * `new Buffer(num)` and `Buffer(num)` will zero-fill new `Buffer` instances
    [[`7eb1b4658e`](7eb1b4658e)]
    [#12141](#12141).
  * Many `Buffer` methods now accept `Uint8Array` as input
    [[`beca3244e2`](beca3244e2)]
    [#10236](#10236).

* **Child Process**
  * Argument and kill signal validations have been improved
    [[`97a77288ce`](97a77288ce)]
    [#12348](#12348),
    [[`d75fdd96aa`](d75fdd96aa)]
    [#10423](#10423).
  * Child Process methods accept `Uint8Array` as input
    [[`627ecee9ed`](627ecee9ed)]
    [#10653](#10653).

* **Console**
  * Error events emitted when using `console` methods are now supressed.
    [[`f18e08d820`](f18e08d820)]
    [#9744](#9744).

* **Dependencies**
  * The npm client has been updated to 5.0.0
    [[`3c3b36af0f`](3c3b36af0f)]
    [#12936](#12936).
  * V8 has been updated to 5.8 with forward ABI stability to 6.0
    [[`60d1aac8d2`](60d1aac8d2)]
    [#12784](#12784).

* **Domains**
  * Native `Promise` instances are now `Domain` aware
    [[`84dabe8373`](84dabe8373)]
    [#12489](#12489).

* **Errors**
  * We have started assigning static error codes to errors generated by Node.js.
    This has been done through multiple commits and is still a work in
    progress.

* **File System**
  * The utility class `fs.SyncWriteStream` has been deprecated
    [[`7a55e34ef4`](7a55e34ef4)]
    [#10467](#10467).
  * The deprecated `fs.read()` string interface has been removed
    [[`3c2a9361ff`](3c2a9361ff)]
    [#9683](#9683).

* **HTTP**
  * Improved support for userland implemented Agents
    [[`90403dd1d0`](90403dd1d0)]
    [#11567](#11567).
  * Outgoing Cookie headers are concatenated into a single string
    [[`d3480776c7`](d3480776c7)]
    [#11259](#11259).
  * The `httpResponse.writeHeader()` method has been deprecated
    [[`fb71ba4921`](fb71ba4921)]
    [#11355](#11355).
  * New methods for accessing HTTP headers have been added to `OutgoingMessage`
    [[`3e6f1032a4`](3e6f1032a4)]
    [#10805](#10805).

* **Lib**
  * All deprecation messages have been assigned static identifiers
    [[`5de3cf099c`](5de3cf099c)]
    [#10116](#10116).
  * The legacy `linkedlist` module has been removed
    [[`84a23391f6`](84a23391f6)]
    [#12113](#12113).

* **N-API**
  * Experimental support for the new N-API API has been added
    [[`56e881d0b0`](56e881d0b0)]
    [#11975](#11975).

* **Process**
  * Process warning output can be redirected to a file using the
    `--redirect-warnings` command-line argument
    [[`03e89b3ff2`](03e89b3ff2)]
    [#10116](#10116).
  * Process warnings may now include additional detail
    [[`dd20e68b0f`](dd20e68b0f)]
    [#12725](#12725).

* **REPL**
  * REPL magic mode has been deprecated
    [[`3f27f02da0`](3f27f02da0)]
    [#11599](#11599).

* **Src**
  * `NODE_MODULE_VERSION` has been updated to 57
    (ec7cbaf266)]
    [#12995](#12995).
  * Add `--pending-deprecation` command-line argument and
    `NODE_PENDING_DEPRECATION` environment variable
    [[`a16b570f8c`](a16b570f8c)]
    [#11968](#11968).
  * The `--debug` command-line argument has been deprecated. Note that
    using `--debug` will enable the *new* Inspector-based debug protocol
    as the legacy Debugger protocol previously used by Node.js has been
    removed. [[`010f864426`](010f864426)]
    [#12949](#12949).
  * Throw when the `-c` and `-e` command-line arguments are used at the same
    time [[`a5f91ab230`](a5f91ab230)]
    [#11689](#11689).
  * Throw when the `--use-bundled-ca` and `--use-openssl-ca` command-line
    arguments are used at the same time.
    [[`8a7db9d4b5`](8a7db9d4b5)]
    [#12087](#12087).

* **Stream**
  * `Stream` now supports `destroy()` and `_destroy()` APIs
    [[`b6e1d22fa6`](b6e1d22fa6)]
    [#12925](#12925).
  * `Stream` now supports the `_final()` API
    [[`07c7f198db`](07c7f198db)]
    [#12828](#12828).

* **TLS**
  * The `rejectUnauthorized` option now defaults to `true`
    [[`348cc80a3c`](348cc80a3c)]
    [#5923](#5923).
  * The `tls.createSecurePair()` API now emits a runtime deprecation
    [[`a2ae08999b`](a2ae08999b)]
    [#11349](#11349).
  * A runtime deprecation will now be emitted when `dhparam` is less than
    2048 bits [[`d523eb9c40`](d523eb9c40)]
    [#11447](#11447).

* **URL**
  * The WHATWG URL implementation is now a fully-supported Node.js API
    [[`d080ead0f9`](d080ead0f9)]
    [#12710](#12710).

* **Util**
  * `Symbol` keys are now displayed by default when using `util.inspect()`
    [[`5bfd13b81e`](5bfd13b81e)]
    [#9726](#9726).
  * `toJSON` errors will be thrown when formatting `%j`
    [[`455e6f1dd8`](455e6f1dd8)]
    [#11708](#11708).
  * Convert `inspect.styles` and `inspect.colors` to prototype-less objects
    [[`aab0d202f8`](aab0d202f8)]
    [#11624](#11624).
  * The new `util.promisify()` API has been added
    [[`99da8e8e02`](99da8e8e02)]
    [#12442](#12442).

* **Zlib**
  * Support `Uint8Array` in Zlib convenience methods
    [[`91383e47fd`](91383e47fd)]
    [#12001](#12001).
  * Zlib errors now use `RangeError` and `TypeError` consistently
    [[`b514bd231e`](b514bd231e)]
    [#11391](#11391).
jasnell added a commit that referenced this pull request May 30, 2017
* **Async Hooks**
  * The `async_hooks` module has landed in core
    [[`4a7233c178`](4a7233c178)]
    [#12892](#12892).

* **Buffer**
  * Using the `--pending-deprecation` flag will cause Node.js to emit a
    deprecation warning when using `new Buffer(num)` or `Buffer(num)`.
    [[`d2d32ea5a2`](d2d32ea5a2)]
    [#11968](#11968).
  * `new Buffer(num)` and `Buffer(num)` will zero-fill new `Buffer` instances
    [[`7eb1b4658e`](7eb1b4658e)]
    [#12141](#12141).
  * Many `Buffer` methods now accept `Uint8Array` as input
    [[`beca3244e2`](beca3244e2)]
    [#10236](#10236).

* **Child Process**
  * Argument and kill signal validations have been improved
    [[`97a77288ce`](97a77288ce)]
    [#12348](#12348),
    [[`d75fdd96aa`](d75fdd96aa)]
    [#10423](#10423).
  * Child Process methods accept `Uint8Array` as input
    [[`627ecee9ed`](627ecee9ed)]
    [#10653](#10653).

* **Console**
  * Error events emitted when using `console` methods are now supressed.
    [[`f18e08d820`](f18e08d820)]
    [#9744](#9744).

* **Dependencies**
  * The npm client has been updated to 5.0.0
    [[`3c3b36af0f`](3c3b36af0f)]
    [#12936](#12936).
  * V8 has been updated to 5.8 with forward ABI stability to 6.0
    [[`60d1aac8d2`](60d1aac8d2)]
    [#12784](#12784).

* **Domains**
  * Native `Promise` instances are now `Domain` aware
    [[`84dabe8373`](84dabe8373)]
    [#12489](#12489).

* **Errors**
  * We have started assigning static error codes to errors generated by Node.js.
    This has been done through multiple commits and is still a work in
    progress.

* **File System**
  * The utility class `fs.SyncWriteStream` has been deprecated
    [[`7a55e34ef4`](7a55e34ef4)]
    [#10467](#10467).
  * The deprecated `fs.read()` string interface has been removed
    [[`3c2a9361ff`](3c2a9361ff)]
    [#9683](#9683).

* **HTTP**
  * Improved support for userland implemented Agents
    [[`90403dd1d0`](90403dd1d0)]
    [#11567](#11567).
  * Outgoing Cookie headers are concatenated into a single string
    [[`d3480776c7`](d3480776c7)]
    [#11259](#11259).
  * The `httpResponse.writeHeader()` method has been deprecated
    [[`fb71ba4921`](fb71ba4921)]
    [#11355](#11355).
  * New methods for accessing HTTP headers have been added to `OutgoingMessage`
    [[`3e6f1032a4`](3e6f1032a4)]
    [#10805](#10805).

* **Lib**
  * All deprecation messages have been assigned static identifiers
    [[`5de3cf099c`](5de3cf099c)]
    [#10116](#10116).
  * The legacy `linkedlist` module has been removed
    [[`84a23391f6`](84a23391f6)]
    [#12113](#12113).

* **N-API**
  * Experimental support for the new N-API API has been added
    [[`56e881d0b0`](56e881d0b0)]
    [#11975](#11975).

* **Process**
  * Process warning output can be redirected to a file using the
    `--redirect-warnings` command-line argument
    [[`03e89b3ff2`](03e89b3ff2)]
    [#10116](#10116).
  * Process warnings may now include additional detail
    [[`dd20e68b0f`](dd20e68b0f)]
    [#12725](#12725).

* **REPL**
  * REPL magic mode has been deprecated
    [[`3f27f02da0`](3f27f02da0)]
    [#11599](#11599).

* **Src**
  * `NODE_MODULE_VERSION` has been updated to 57
    (ec7cbaf266)]
    [#12995](#12995).
  * Add `--pending-deprecation` command-line argument and
    `NODE_PENDING_DEPRECATION` environment variable
    [[`a16b570f8c`](a16b570f8c)]
    [#11968](#11968).
  * The `--debug` command-line argument has been deprecated. Note that
    using `--debug` will enable the *new* Inspector-based debug protocol
    as the legacy Debugger protocol previously used by Node.js has been
    removed. [[`010f864426`](010f864426)]
    [#12949](#12949).
  * Throw when the `-c` and `-e` command-line arguments are used at the same
    time [[`a5f91ab230`](a5f91ab230)]
    [#11689](#11689).
  * Throw when the `--use-bundled-ca` and `--use-openssl-ca` command-line
    arguments are used at the same time.
    [[`8a7db9d4b5`](8a7db9d4b5)]
    [#12087](#12087).

* **Stream**
  * `Stream` now supports `destroy()` and `_destroy()` APIs
    [[`b6e1d22fa6`](b6e1d22fa6)]
    [#12925](#12925).
  * `Stream` now supports the `_final()` API
    [[`07c7f198db`](07c7f198db)]
    [#12828](#12828).

* **TLS**
  * The `rejectUnauthorized` option now defaults to `true`
    [[`348cc80a3c`](348cc80a3c)]
    [#5923](#5923).
  * The `tls.createSecurePair()` API now emits a runtime deprecation
    [[`a2ae08999b`](a2ae08999b)]
    [#11349](#11349).
  * A runtime deprecation will now be emitted when `dhparam` is less than
    2048 bits [[`d523eb9c40`](d523eb9c40)]
    [#11447](#11447).

* **URL**
  * The WHATWG URL implementation is now a fully-supported Node.js API
    [[`d080ead0f9`](d080ead0f9)]
    [#12710](#12710).

* **Util**
  * `Symbol` keys are now displayed by default when using `util.inspect()`
    [[`5bfd13b81e`](5bfd13b81e)]
    [#9726](#9726).
  * `toJSON` errors will be thrown when formatting `%j`
    [[`455e6f1dd8`](455e6f1dd8)]
    [#11708](#11708).
  * Convert `inspect.styles` and `inspect.colors` to prototype-less objects
    [[`aab0d202f8`](aab0d202f8)]
    [#11624](#11624).
  * The new `util.promisify()` API has been added
    [[`99da8e8e02`](99da8e8e02)]
    [#12442](#12442).

* **Zlib**
  * Support `Uint8Array` in Zlib convenience methods
    [[`91383e47fd`](91383e47fd)]
    [#12001](#12001).
  * Zlib errors now use `RangeError` and `TypeError` consistently
    [[`b514bd231e`](b514bd231e)]
    [#11391](#11391).
cjihrig added a commit to cjihrig/node that referenced this pull request Jun 6, 2017
This commit removes process._inspectorEnbale which was
spelled incorrectly, and is being properly implemented
in a separate PR.

Refs: nodejs#12949
PR-URL: nodejs#13460
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Luca Maraschi <luca.maraschi@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
jasnell pushed a commit that referenced this pull request Jun 7, 2017
This commit removes process._inspectorEnbale which was
spelled incorrectly, and is being properly implemented
in a separate PR.

Refs: #12949
PR-URL: #13460
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Luca Maraschi <luca.maraschi@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@refack refack removed their assignment Jun 12, 2017
@refack refack mentioned this pull request Jun 12, 2017
3 tasks
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
3 tasks
@MylesBorins
Copy link
Member

Should this be backported to v6.x?

@refack
Copy link
Contributor Author

refack commented Jul 17, 2017

Should this be backported to v6.x?

I'm not sure. Ping @nodejs/diagnostics, @nodejs/release should we / could we replace the:

Warning: This is an experimental feature and could change at any time.

with

(node:13156) [DEP0062] DeprecationWarning: `node --inspect --debug-brk` is deprecated. Please use `node --inspect-brk` instead.

@jkrems
Copy link
Contributor

jkrems commented Jul 18, 2017

(node:13156) [DEP0062] DeprecationWarning: `node --inspect --debug-brk` is deprecated.
Please use `node --inspect-brk` instead.

Did we port --inspect-brk to 6.x yet? If so, then adding that warning sounds good.

@sam-github
Copy link
Contributor

Weird that the PR's file changes are empty, you need to explicitly look at 16689e3 to see what changed, but its a largish diff, with no docs (?), and the commit message doesn't say what it does.

@refack What did this PR do?

Add --debug-brk and --debug back in as an undocumented and deprecated option until 7.x is no longer
supported.

Is that still accurate? 7.x is no longer supported, so if this was a temporary measure to make 8.x and 7.x be more compatible, maybe it doesn't apply to 6.x, and can now be reversed on 8.x? Or on 9.x?

@refack
Copy link
Contributor Author

refack commented Jul 21, 2017

The main point was to make --inspect --debug-brk work again and emit a deprecation warning (while also making lone --debug and --debug-brk invalid):

if (process._invalidDebug) {
  process.emitWarning(
    '`node --debug` and `node --debug-brk` are invalid. ' +
    'Please use `node --inspect` or `node --inspect-brk` instead.',
    'DeprecationWarning', 'DEP0062', startup, true);
  process.exit(9);
} else if (process._deprecatedDebugBrk) {
  process.emitWarning(
    '`node --inspect --debug-brk` is deprecated. ' +
    'Please use `node --inspect-brk` instead.',
    'DeprecationWarning', 'DEP0062', startup, true);
}
Add --debug-brk and --debug back in as an undocumented and deprecated option
until 7.x is no longer supported.

Is that still accurate? 7.x is no longer supported, so if this was a temporary measure to make 8.x and 7.x be more compatible, maybe it doesn't apply to 6.x, and can now be reversed on 8.x? Or on 9.x?

I'm not sure it's accurate, well at least it's only part of the motivation. The other part is that 3rd party tooling depend on the --inspect --debug-brk combo, so IMHO we need to wait at least a year (an arbitrary measure of tools' version lifespan).
Also we declared the combo "valid but deprecated" so it needs to stay within the v8.x line.


As for backporting to v6.x, there the use of any --inspect* argument emits an experimental warning. IMHO that is no longer true, and should be removed and probably replaced with a deprecation warning for --debug*. Anyway emitting both for the --inspect --debug-break combo does not make sense.

@sam-github
Copy link
Contributor

Thanks @refack . I marked as dont-land-on-v6.x. We could rearrange the warnings, etc., for inspect/debug on 6.x but in the absence of complaints, it just pokes a hornet's nest of questions. Making --debug emit new deprecation messages is semver-major, so it can't happen. Removing experimental warning for inspect could be considered semver-major, too.

@refack
Copy link
Contributor Author

refack commented Jul 21, 2017

Thanks @refack . I marked as dont-land-on-v6.x. We could rearrange the warnings, etc., for inspect/debug on 6.x but in the absence of complaints, it just pokes a hornet's nest of questions. Making --debug emit new deprecation messages is semver-major, so it can't happen. Removing experimental warning for inspect could be considered semver-major, too.

Sure, makes sense

@gibfahn
Copy link
Member

gibfahn commented Jul 22, 2017

Is that still accurate? 7.x is no longer supported, so if this was a temporary measure to make 8.x and 7.x be more compatible, maybe it doesn't apply to 6.x, and can now be reversed on 8.x? Or on 9.x?

I think the plan was to drop support for --debug-brk in a future release. I'd be fine with it happening in v9.x, @refack are you suggesting we wait till v10.x?

@refack
Copy link
Contributor Author

refack commented Jul 22, 2017

@refack are you suggesting we wait till v10.x?

Yes.
To be clear: should be in 9, should not be in 10.
I would summon all the 3rd party stakeholders, but I think we can agree on this, and ping them when we do remove it.

@jkrems
Copy link
Contributor

jkrems commented Jul 22, 2017

I would summon all the 3rd party stakeholders.

👍 to that. I think we saw that being a bit more conservative with changing this interface is healthy. Given that we have a chance to remove it every few month and (afaict) it's not causing a huge maintenance burden, there's no real pressure to remove it. Let's not forget that the latest LTS still requires all tools to use --inspect --debug-brk, so they couldn't even start using --inspect-brk realistically.

@gibfahn
Copy link
Member

gibfahn commented Jul 23, 2017

Happy to wait till 10.x.

@segrey
Copy link

segrey commented Jan 24, 2018

For some configurations IDE still uses --inspect --debug-brk combo without version detection: e.g. it's so for remote Node.js interpreters. It allows to target wide version range: node 6 (>= 6.3.0), 7, 8, 9 and node 10 nightly (tested on https://nodejs.org/download/nightly/v10.0.0-nightly201801239870b53810/). There will be problems if support for --inspect --debug-brk is dropped in node 10.x as it was planned.
@refack Is it possible to preserve --inspect --debug-brk in node 10 and future node versions until node 6 maintenance LTS ends (April 2019 according to https://github.com/nodejs/Release)? // cc @ulitink

joyeecheung added a commit that referenced this pull request Feb 1, 2019
Moves the exit of `--debug` and `--debug-brk` earlier, that is,
after the option parsing is done in the C++ land.

Also removes `process._invalidDebug`.

PR-URL: #25828
Refs: #12949
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
joyeecheung added a commit that referenced this pull request Feb 1, 2019
This has already been practically end-of-life since `node --debug`
alone would exit the process. This patch drops support of
`node --inspect --debug-brk` as well.

`node --inspect --debug-brk` has been deprecated since v8,
it has been maintained so that vendors can target Node.js
v6 and above without detecting versions.
The support of `--inspect`, which starts from v6, will reach
end-of-life in April 2019, it should be safe to drop the support
of `--inspect --debug-brk` altogether in v12.

Also removes `process._deprecatedDebugBrk`

PR-URL: #25828
Refs: #12949
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. cli Issues and PRs related to the Node.js command line interface. inspector Issues and PRs related to the V8 inspector protocol
Projects
None yet
10 participants