-
Notifications
You must be signed in to change notification settings - Fork 29k
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
error: document removed error codes #22100
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some initial nits.
doc/api/errors.md
Outdated
<!-- YAML | ||
added: v10.0.0 | ||
--> | ||
An attempt was made to start a watcher returned by `fs.watch()` that has already been started. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Linter issue: 80 characters per line limit exceeded.
doc/api/errors.md
Outdated
added: v10.0.0 | ||
--> | ||
|
||
An attempt was made to initiate operations on a watcher returned by `fs.watch()` that has not yet been started. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Linter issue: 80 characters per line limit exceeded.
doc/api/errors.md
Outdated
@@ -1890,6 +1905,7 @@ A module file could not be resolved while attempting a [`require()`][] or | |||
[`zlib`]: zlib.html | |||
[ES6 module]: esm.html | |||
[Node.js Error Codes]: #nodejs-error-codes | |||
[Legacy Node.js Error Codes]: #legacy-nodejs-error-codes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Linter issue: missing heading for this link?
- Doc tools use underscores, not hyphens in auto-created section ids, with document name prefix:
#errors_legacy_node_js_error_codes
for## Legacy Node.js Error Codes
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doh! missed adding the h2 header, fixing it now. Also, realized that I don't really need the link definition at the end of the file, so removed it.
1444f20
to
cbe275c
Compare
doc/api/errors.md
Outdated
@@ -1847,20 +1847,25 @@ Creation of a [`zlib`][] object failed due to incorrect configuration. | |||
A module file could not be resolved while attempting a [`require()`][] or | |||
`import` operation. | |||
|
|||
<a id="legacy-nodejs-error-codes"></a> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I may be wrong, but we may need not this hardcoded id, as the other ones in this doc may be due to legacy links support. Let us see what others think.
doc/api/errors.md
Outdated
<a id="ERR_FS_WATCHER_ALREADY_STARTED"></a> | ||
### ERR_FS_WATCHER_ALREADY_STARTED | ||
<!-- YAML | ||
added: v10.0.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice if we add a "removed: v10.x.x". I guess that might not be parsed properly tough. @vsemozhetbyt do you have a suggestion for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@BridgeAR added these sections (thanks @vsemozhetbyt, for the pointers), now shows up as
Although cc: @joyeecheung , because it seems to me she has some reservations against saying "removed in v10.x.x" .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, should there be a short explanation on why this error code was removed? Something on the lines of
Like
fs.watchFile()
, this situation is now silently ignored.
or some such.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SirR4T I think it's OK to have removed in
, but the version number should be accurate. In the case of ERR_FS_WTCHER_ALREADY_STARTED
, it has never been released in any version, since the commit that added it (6c25f2e) and the commit that removed it (301f6cc) are both present from v10.0.0...v10.8.0 - which means when v10.0.0 was out, the error was not even there. Therefore, this error code has only appeared in nightly and canary releases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we do add a short explanation, we can make this in changes
blocks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
got it. If the error codes never actually touched the releases, what should the description look like? is having both added: v10.0.0
and removed: v10.0.0
ok?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joyeecheung : also, I have added versions added and versions removed here, for all the commits which either added or removed the error codes. Eyeballing the versions, it seems to me that the wherever github mentions vA.a.a .. vB.b.b
, we should be mentioning the vB.b.b
in the docs. (For both added, as well as removed.) will that be correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think annotating them as added: v10.0.0 removed: v10.0.0
is more confusing than helpful. A note in the docs about that would be enough, maybe something like:
This error code has never been released and only existed in nightly builds.
218bc9e
to
0c11e4d
Compare
Intermediate CI-lite: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/451/ |
if someone can confirm this looks good, I'll continue adding the other error codes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doc format and doctools additions LGTM.
additional info, could be useful.
|
dcc5a1b
to
dd31ede
Compare
doc/api/errors.md
Outdated
removed: v10.0.0 | ||
--> | ||
|
||
HTTP/2 Informational headers must only be sent *prior* to calling the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Informational
-> informational
(lower case).
doc/api/errors.md
Outdated
removed: REPLACEME | ||
--> | ||
|
||
Used when `hostname` can not be parsed from a provided URL. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can not
-> cannot
doc/api/errors.md
Outdated
removed: v10.0.0 | ||
--> | ||
|
||
The `repl` module was unable parse data from the REPL history file. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unable parse
-> unable to parse
doc/api/errors.md
Outdated
|
||
Used to prevent an abort if a string decoder was set on the Socket. | ||
|
||
Example |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe omit the word Example
as obvious.
doc/api/errors.md
Outdated
const Socket = require('net').Socket; | ||
const instance = new Socket(); | ||
|
||
instance.setEncoding('utf-8'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'utf-8'
-> 'utf8'
Thanks for digging into this! On how to document the
|
Documentation can be added once the PRs land on a release. Do not add documentation for those error codes yet. [See point 2 here](nodejs#22100 (comment)).
@vsemozhetbyt : regarding 1., I believe current YAML for doc sections only supports added, removed, and changes. Any idea on how to add the regarding 2., have removed docs for the two error codes ( regarding 3., yay! I was on track. |
@SirR4T As |
I think we can just skip those? If they are not even released there is no point adding those fields. A note in the docs would be enough. |
@joyeecheung @vsemozhetbyt Have moved these errors to another list, as I wasn't sure how to add a note. Lemme know if this looks good.. |
doc/api/errors.md
Outdated
@@ -2073,6 +1990,64 @@ removed: v10.0.0 | |||
Used when an attempt is made to use a `zlib` object after it has already been | |||
closed. | |||
|
|||
<a id="unreleased_error_codes"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems this anchor is unneeded (as well as the <a id="legacy-nodejs-error-codes"></a>
above). We use hardcoded anchors only for error headings in this doc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done. Also, any other suggestions welcome, for the last section description. The current These errors have never been released, but had been present on master between releases.
seems to assume knowledge of the git repo on the behalf of a reader.
It seems like a good solution for me. |
@@ -34,6 +34,10 @@ function extractAndParseYAML(text) { | |||
meta.deprecated = arrify(meta.deprecated); | |||
} | |||
|
|||
if (meta.removed) { | |||
meta.removed = arrify(meta.removed); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this needed for this PR? From what I can tell we only used single removed
fields here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was blindly copy pasting wherever added
occurred.
Maybe not for this particular PR, but if we're planning to add removed
as a feature for api docs, would rather it supports semver-minors as well, like added
and deprecated
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SirR4T Ah, sorry, I thought we already had removed
before, looks like it is new in this PR...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work, thanks for picking this up!
One note: this should be landed as two commits, one for the doc change and one for the tools change
PR-URL: #22100 Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Landed in e10290c...e9876fd |
PR-URL: #22100 Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
PR-URL: #22100 Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
PR-URL: #22100 Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesWorking towards #22061
List of errors to be documented as removed (from #21491)
ERR_FS_WATCHER_ALREADY_STARTED
ERR_FS_WATCHER_NOT_STARTED
ERR_HTTP2_ALREADY_SHUTDOWN
ERR_HTTP2_ERROR
ERR_HTTP2_FRAME_ERROR
ERR_HTTP2_HEADERS_OBJECT
ERR_HTTP2_HEADER_REQUIRED
ERR_HTTP2_INFO_HEADERS_AFTER_RESPOND
ERR_HTTP2_STREAM_CLOSED
ERR_HTTP_INVALID_CHAR
ERR_INVALID_ARRAY_LENGTH
ERR_INVALID_DOMAIN_NAME
ERR_INVALID_REPL_HISTORY
ERR_MISSING_DYNAMIC_INSTANTIATE_HOOK
ERR_NAPI_CONS_PROTOTYPE_OBJECT
ERR_OUTOFMEMORY
ERR_PARSE_HISTORY_DATA
ERR_STREAM_HAS_STRINGDECODER
ERR_STREAM_READ_NOT_IMPLEMENTED
ERR_STRING_TOO_LARGE
ERR_TLS_RENEGOTIATION_FAILED
ERR_UNKNOWN_BUILTIN_MODULE
ERR_VALUE_OUT_OF_RANGE
ERR_ZLIB_BINDING_CLOSED