- Instead of storing a pointer whose type refers to the specific
subclass of `BaseObject`, just store a `BaseObject*` directly.
This means in particular that one can cast to classes along
the way of the inheritance chain without issues, and that
`BaseObject*` no longer needs to be the first superclass
in the case of multiple inheritance.
In particular, this renders hack-y solutions to this problem (like
ddc19be6de) obsolete and addresses
a `TODO` comment of mine.
- Move wrapping/unwrapping methods to the `BaseObject` class.
We use these almost exclusively for `BaseObject`s, and I hope
that this gives a better idea of how (and for what) these are used
in our code.
- Perform initialization/deinitialization of the internal field
in the `BaseObject*` constructor/destructor. This makes the code
a bit more obviously correct, avoids explicit calls for this
in subclass constructors, and in particular allows us to avoid
crash situations when we previously called `ClearWrap()`
during GC.
This also means that we enforce that the object passed to the
`BaseObject` constructor needs to have an internal field.
This is the only reason for the test change.
- Change the signature of `MakeWeak()` to not require a pointer
argument. Previously, this would always have been the same
as `this`, and no other value made sense. Also, the parameter
was something that I personally found somewhat confusing
when becoming familiar with Node’s code.
- Add a `TODO` comment that motivates switching to real inheritance
for the JS types we expose from the native side. This patch
brings us a lot closer to being able to do that.
- Some less significant drive-by cleanup.
Since we *effectively* already store the `BaseObject*` pointer
anyway since ddc19be6de, I do not
think that this is going to have any impact on diagnostic tooling.
Fixes: https://github.com/nodejs/node/issues/18897
PR-URL: https://github.com/nodejs/node/pull/20455
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
This was originally introduced in 3446ff417b, in order to fix
a hard crash. However, since the libuv 1.18.0 update, that hard
crash is gone, and since f2b9805f85 we do not throw an
error in JS land anymore either, rendering the flag unnecessary.
Also, the original test that checked this condition was added
to `test/parallel/`. Since that typically runs without a TTY stdin,
a duplicate test is being added to the pseudo-tty test suite
in this commit.
Refs: 3446ff417b
Refs: f2b9805f85
Refs: 0e2814179c
PR-URL: https://github.com/nodejs/node/pull/20388
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
In pull-requests.md:
* Refer to the Collaborator Guide as Collaborator Guide and not
Collaborator's Guide. That is how the doc describes itself and we
should be consistent.
PR-URL: https://github.com/nodejs/node/pull/20473
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
The documentation used `file` for the first argument to `appendFile()`
functions. However, the code and (more importantly) thrown errors
referred to it as `path`. The latter is especially important because
context is not provided. So you're looking for a function that takes
`path` but that string doesn't appear in your code *or* in the
documentation. It's not until the end user looks at the source code of
Node.js that they can figure out what's going on. This is why it is
important that the names of variables in the documentation match that in
the code. If we want to change this to `file`, then that's OK, but we
need to do it in the source code and error messages too, not just in the
docs. Changing the docs is the smallest change to synchronize everything
so that's what this change does.
PR-URL: https://github.com/nodejs/node/pull/20489
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Do less variable allocations and reassignments inside spliceOne
since it's relied on by some performance sensitive code.
PR-URL: https://github.com/nodejs/node/pull/20453
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Instead of always checking whether we've already warned about a
possible EventEmitter memory leak, first run the rest of the
code as accessing random properties on an Array is expensive.
In addition, remove an unnecessary truthy check.
PR-URL: https://github.com/nodejs/node/pull/20452
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Use the potentially more efficient fs.copyFileSync() instead of reading
the whole file in and writing the whole file out in JavaScript.
PR-URL: https://github.com/nodejs/node/pull/20340
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
In http.ClientRequest's doc, add maxHeadersCount as a public property.
And in the description of server's one, change a hyphen to a comma.
PR-URL: https://github.com/nodejs/node/pull/20361
Refs: https://github.com/nodejs/node/pull/20359
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
A recent change made these benchmarks fail by always finishing
with 0 iterations. Restore a counter variable.
PR-URL: https://github.com/nodejs/node/pull/20461
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Tell the contributor to generally not squash commits during the pull
request review process.
PR-URL: https://github.com/nodejs/node/pull/20413
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Although I agree with the guideline people should generally not squash
commits in a pull request until the end (in other words, until it's time
to land the PR), it is clear from comments and actions in the issue
tracker that many do not share that view. This is fine by me, but I do
think that we should our documentation should reflect our practices
rather than being an aspirational statement.
If we *do* wish to preserve this recommendation, it probably belongs in
another document anyway as this is not a recommendation for
Collaborators only but for anyone opening a pull request.
PR-URL: https://github.com/nodejs/node/pull/20413
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Original commit message:
Fix Object.entries/.values with non-enumerable properties
Iterate over all descriptors instead of bailing out early and missing
enumerable properties later.
Bug: chromium:836145
Change-Id: I104f7ea89480383b6b4b9204942a166bdf8e0597
Reviewed-on: https://chromium-review.googlesource.com/1027832
Reviewed-by: Jakob Gruber <jgruber@chromium.org>
Commit-Queue: Camillo Bruni <cbruni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#52786}
Refs: 76cab5ff78
Fixes: https://github.com/nodejs/node/issues/20278
PR-URL: https://github.com/nodejs/node/pull/20350
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
This commit removes unnecesary requires of http and internal/util in
http2/core.js
PR-URL: https://github.com/nodejs/node/pull/20422
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
This commit removes v8 qualified identifiers that have using directives
to be consistent with the rest of the code in node_http2.cc.
PR-URL: https://github.com/nodejs/node/pull/20420
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Some other formatting nits were fixed
and some superfluous descriptions were simplified in passing.
PR-URL: https://github.com/nodejs/node/pull/20401
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
We cannot manage with `{any}` because `message` cannot be
of `Symbol` or `undefined` types.
PR-URL: https://github.com/nodejs/node/pull/20405
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
There is no need to get this property twice in this rather hot spot:
if there is no such key, the `typeUrl` will be `undefined`,
which suffices for the boolean check in the next line.
For consistency, `undefined` can also be made the default value.
PR-URL: https://github.com/nodejs/node/pull/20387
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
There is a paragraph about why the promise-based API uses `FileHandle`
rather than a numeric `fd`. Unfortunately, the paragraph is a bit of
word-salad. Edit it for clarity, grammar, and style.
PR-URL: https://github.com/nodejs/node/pull/20450
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
There is a paragraph explaining that `fs.write()` cannot write a
substring of the provided string. It's not clear what `buffer` refers
to in that paragraph or even what the purpose of the paragraph is.
There's no suggestion elsewhere that `fs.write()` should be expected to
have a substring feature. Remove the paragraph.
PR-URL: https://github.com/nodejs/node/pull/20450
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Shorten the text about the `highWaterMark` value for the stream returned
by `fs.createReadStream()`.
PR-URL: https://github.com/nodejs/node/pull/20450
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Since the table of contents is built dynamically when
building the documentation, the additional table of
contents is redundant and an extra overhead to maintain.
PR-URL: https://github.com/nodejs/node/pull/20395
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Add parameters for the callback for the Http2Session:localSettings event
and Http2Session:remoteSettings event inline with the pattern in the
rest of the documentation.
Refs: https://github.com/nodejs/help/issues/877#issuecomment-381253464
PR-URL: https://github.com/nodejs/node/pull/20371
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Remove unnecessary "has been known to" tentativeness from
COLLABORATOR_GUIDE.md. "has been known to" is an awkward replacement for
"can" or "might" or "sometimes does" or "does". Pick the right one and
use it.
PR-URL: https://github.com/nodejs/node/pull/20412
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
This removes the former default values and the spread arguments
usage. That was unnecessary and now it does only what is necessary.
The `message` function got renamed to `getMessage` to outline that
it is actually a function and a helper function was inlined into
the SystemError constructor as it was only used there.
PR-URL: https://github.com/nodejs/node/pull/20337
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Add test that confirms that when libuv reports a memory error on a DNS
query, that the memory error is passed through and not replaced with
ENOTFOUND.
PR-URL: https://github.com/nodejs/node/pull/20317
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
This addresses a part of a TODO by properly reporting an out of
memory error to the user instead of reporting `ENOTFOUND`.
PR-URL: https://github.com/nodejs/node/pull/20317
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Remove "(I prefer the former)" from onboarding-extras. Without any
explanation as to why one might prefer A over B, it probably raises more
questions than it answers.
PR-URL: https://github.com/nodejs/node/pull/20393
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
