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 — process: Add code to warnings, assign codes to deprecations by jasnell · Pull Request #10116 · nodejs/node Conversation
|
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Why “only if the file does not already exist”? I’m not sure I’d expect that, and it sounds a bit like that might interfere with programmatic use of the API.
Is this about multiple node processes sharing the same env var?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I'm not entirely sure about the only if the file does not already exist part either. For now I have it this way just to ensure that something important isn't accidentally overwritten.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should just append?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Just appending works also. Doing so is pretty safe when dealing with multiple node processes since the warning output includes the pid
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
This should probably be \_linklist (including the \)
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I’m not sure the semantic difference between name and code is obvious here?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Name may be better as "type"?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
type is a good option
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Name may be better as "type"?
Remember that .name is inherited from the Error classes – I don’t think we can change that.
I’m also not worried that much about the naming, a bit of clarification might be all that’s needed
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
The argument can be called type with name still used on the error object. I'm not overly concerned about the difference there.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
nit: Runtime deprecation? ;)
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
(ditto)
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Nit: fd === undefined
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
nit: name !== undefined and code !== undefined
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
(ditto)
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
nit: nullptr
Sorry, something went wrong.
There was an error while loading. Please reload this page.
5bf38a7 to
3b98c43
Compare
|
@addaleax @Fishrock123 ... updated to address the nits. Updated the |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I think a options object {type, code, ctor} is in order here.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I think this is basically just intended for internal use, so doesn't need a nice API, and changing the call signature would cause a fair amount of internal churn.
Honestly, I don't think it should be an API at all, maybe we can deprecate it? Seriously, why do we want to expose this to users of node? We expect them to be using it to emit their own warnings?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Honestly, I don't think it should be an API at all, maybe we can deprecate it? Seriously, why do we want to expose this to users of node? We expect them to be using it to emit their own warnings?
Fwiw it was deliberately added as an outward-facing API in c6656db (released in v6.0.0) with that intention, yes. I don’t mind it being there.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
It was exposed this way because we had users asking to be able to use it also. I'm definitely -1 on deprecating it as I have found it to be quite useful
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
OK, then I agree with @silverwind, its become an ugly API, with three strings in a row, 2 of which are optional, and remembering the arg order isn't reasonably possible. @addaleax the problem with APIs is they rot with time, becoming harder and harder to use and support, like this one is now, which is why I am generally in favour of not adding unnecessary APIs, even if when they are useful. I guess given the alternative of adding a second options-based call pattern, and supporting the old 3 argument call for backwards compatibility, and documenting both of them, or just adding another optional string arg, better to do the latter. If we ever add a 4th string, though, it will no longer be reasonable.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
@sam-github My thoughts exactly. Supporting two call signatures is easy and enables easier future additons.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
The API already supports two call signatures. The first argument can be passed in as a pre-constructed Error object with all of the relevant properties set:
const warning = new Error('This is a warning'); warning.name = 'CustomWarning'; warning.code = 'ABC123'; process.emitWarning(warning);
is equivalent to:
process.emitWarning('This is a warning', 'CustomWarning', 'ABC123');
Having an options object when we can already pass in an Error object would be rather pointless, IMO
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
That Error behaviour isn't well documented. I read carefully, and I see now that its implied by by the "If warning is passed as an Error object, it will be passed through .... unmodified" sentence.
I think the docs could be clearer, but that's an aside from this PR.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
It's not just implied, it's explicitly stated in the arguments list that warning can be a string or an Error, and an example of passing an Error is shown immediately following the If warning is passed as an Error object... sentence. Immediately following the example is a clear statement that a TypeError will be thrown is warning is anything other than a string or Error object. The docs seem pretty explicit about it.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
I disagree. The method already has the option of passing in a Error object
as the first parameter. Switching to an options object would just add
confusion, I think.
…On Sun, Dec 4, 2016 at 4:44 PM silverwind ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In doc/api/process.md
<#10116 (review)>:
> @@ -706,14 +706,15 @@ console.log(process.env.test);
// => 1
```
-## process.emitWarning(warning[, name][, ctor])
+## process.emitWarning(warning[, type[, code]][, ctor])
I think a options object is in order here.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#10116 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAa2ec-j0kNP2A0JnuBeKN3ejbShHNY-ks5rE15ugaJpZM4LDsqo>
.
|
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Buffer() is not considered as a constructor
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Providing links to these methods will be helpful.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
use
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
built-in
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
We explicitly ask the readers to refer documentation, it would be better if we had the link here.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
property or API
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
doc-deprecated?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Should there be a space after ${prefix}?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
No, the ${prefix} includes a space already
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Can we split them into multiple variables?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
assert.ifError
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
It's more of a general adivce to design APIs in a extendable way. I think that once a function takes 3 or more arguments, it gets to a point where the user has to lookup the arguments on less-frequently used functions, which could've been avoided if it would be designed with a options object from the start. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
What do we do about things we have to un-deprecate? Just remove them from the doc? Or mark them as no longer deprecated? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
I'd leave them in the doc and label them as undeprecated. The codes should
increase monotonically so leaving them in the doc will avoid collisions
…On Mon, Dec 5, 2016 at 7:10 AM Jeremiah Senkpiel ***@***.***> wrote:
What do we do about things we have to un-deprecate? Just remove them from
the doc? Or mark them as no longer deprecated?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#10116 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAa2eWrYaP8LINcM1uUgqt8HFsZHXYIaks5rFClXgaJpZM4LDsqo>
.
|
Sorry, something went wrong.
There was an error while loading. Please reload this page.
d6799a0 to
eea42cb
Compare
|
@silverwind ... I don't disagree, but as I said, this API already provides the option of passing in an |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@thefourtheye ... updated to address your feedback. I squashed and force pushed to keep things organized. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
658588a to
e5d79fd
Compare
|
New CI: https://ci.nodejs.org/job/node-test-pull-request/5211/ |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
e5d79fd to
3a19ece
Compare
|
@nodejs/ctc ... thoughts on this? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@jasnell there a way to automatically track the next deprecation code? feel it could be error prone or frustrating to grep all the current ones to find the next. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
There might be, but we need to make sure they stay static once assigned.
Best bet would be to check the deprecations.md and assign the next highest,
then make sure that doc always gets updated with a new deprecation
…On Fri, Dec 9, 2016 at 2:11 PM Trevor Norris ***@***.***> wrote:
@jasnell <https://github.com/jasnell> there a way to automatically track
the next deprecation code? feel it could be error prone or frustrating to
grep all the current ones to find the next.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#10116 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAa2efCdwqCpSxTV1JESmkYNkFF7MQHwks5rGdIEgaJpZM4LDsqo>
.
|
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@jasnell ah. missed |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@targos updated! PTAL |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Add the ability to assign an optional code to process warnings + add additional type checking to ensure that names and codes can only be strings.
The --redirect-warnings command line argument allows process warnings to be written to a specified file rather than printed to stderr. Also adds an equivalent NODE_REDIRECT_WARNINGS environment variable. If the specified file cannot be opened or written to for any reason, the argument is ignored and the warning is printed to stderr. If the file already exists, it will be appended to.
Assigns a static identifier code to all runtime and documentation only deprecations. The identifier code is included in the emitted DeprecationWarning. Also adds a deprecations.md to the API docs to provide a central location where deprecation codes can be referenced and explained.
b08dcb2 to
ab50285
Compare
Add the ability to assign an optional code to process warnings + add additional type checking to ensure that names and codes can only be strings. PR-URL: #10116 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Michal Zasso <[email protected]> Reviewed-By: Fedor Indutny <[email protected]>
The --redirect-warnings command line argument allows process warnings to be written to a specified file rather than printed to stderr. Also adds an equivalent NODE_REDIRECT_WARNINGS environment variable. If the specified file cannot be opened or written to for any reason, the argument is ignored and the warning is printed to stderr. If the file already exists, it will be appended to. PR-URL: #10116 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Michal Zasso <[email protected]> Reviewed-By: Fedor Indutny <[email protected]>
Assigns a static identifier code to all runtime and documentation only deprecations. The identifier code is included in the emitted DeprecationWarning. Also adds a deprecations.md to the API docs to provide a central location where deprecation codes can be referenced and explained. PR-URL: #10116 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Michal Zasso <[email protected]> Reviewed-By: Fedor Indutny <[email protected]>
There was an error while loading. Please reload this page.
|
Landed in 5e1f32f...5de3cf0 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@jasnell do you plan to backport this? Its a useful feature, and it appears to not be necessarily semver-major:
Its blocking later PRs that touch nearby code even though they don't depend on this feature, like #11051, and its follow ons. I can backport if you don't have time, but would like it in 7.x. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
I backported it to 4.x today and opened a PR. It should be fairly trivial to backport to 7.x and 6.x. I wouldn't be able to get to it until next week so if you'd like to go for it, be my guest. btw... I would also include the new |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
* **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).
|
I backported 03e89b3 to 6.x, the other commits look semver-major to me, as they add deprecation code and change the output format. Do I misunderstand? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.