src: add receiver to fast api callback methods#54408

Ceres6 · 2024-08-16T11:17:33Z

When creating an fast api the callback might use the receiver. In that case if the internal binding is destructured the method won't have access to the reciver and it will throw. Passing the receiver as second argument ensures the receiver is available.

nodejs-github-bot · 2024-08-16T11:17:39Z

Review requested:

@nodejs/loaders
@nodejs/security-wg
@nodejs/tsc

Ceres6 · 2024-08-16T11:23:07Z

cc @joyeecheung @anonrig

codecov · 2024-08-16T12:30:01Z

Codecov Report

Attention: Patch coverage is 50.00000% with 12 lines in your changes missing coverage. Please review.

Project coverage is 88.23%. Comparing base (2c14615) to head (41522d4).
Report is 822 commits behind head on main.

Files with missing lines	Patch %	Lines
src/histogram.cc	42.85%	4 Missing ⚠️
src/timers.cc	0.00%	4 Missing ⚠️
src/node_process.h	0.00%	2 Missing ⚠️
lib/internal/modules/esm/resolve.js	80.00%	0 Missing and 1 partial ⚠️
src/node_wasi.cc	0.00%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #54408 +/- ## ========================================== + Coverage 87.08% 88.23% +1.15%  ========================================== Files 648 651 +3 Lines 182341 183881 +1540 Branches 34982 35863 +881 ========================================== + Hits 158783 162252 +3469 + Misses 16831 14920 -1911 + Partials 6727 6709 -18

Files with missing lines	Coverage Δ
lib/fs.js	`98.15% <100.00%> (+4.88%)`	⬆️
lib/internal/fs/promises.js	`98.24% <100.00%> (+0.68%)`	⬆️
lib/internal/modules/cjs/loader.js	`97.37% <100.00%> (+4.44%)`	⬆️
src/histogram.h	`60.00% <ø> (ø)`
src/node_external_reference.h	`100.00% <ø> (ø)`
src/node_file.cc	`76.99% <100.00%> (+0.10%)`	⬆️
src/node_wasi.h	`0.00% <ø> (ø)`
src/timers.h	`100.00% <ø> (ø)`
lib/internal/modules/esm/resolve.js	`96.54% <80.00%> (+0.09%)`	⬆️
src/node_wasi.cc	`65.36% <0.00%> (-1.79%)`	⬇️
... and 3 more

... and 241 files with indirect coverage changes

joyeecheung

LGTM with doc nits

doc/contributing/adding-v8-fast-api.md

Co-authored-by: Joyee Cheung <[email protected]>

doc/contributing/adding-v8-fast-api.md

nodejs-github-bot · 2024-09-24T18:58:54Z

CI: https://ci.nodejs.org/job/node-test-pull-request/62743/

Co-authored-by: Aviv Keller <[email protected]>

Ceres6 · 2024-09-25T07:10:34Z

The failing tests seem to be in the flaky list except pummel.test-hash-seed which might be a flake too. Should we re-run?

nodejs-github-bot · 2024-09-27T08:51:19Z

CI: https://ci.nodejs.org/job/node-test-pull-request/62797/

nodejs-github-bot · 2024-09-27T14:00:42Z

CI: https://ci.nodejs.org/job/node-test-pull-request/62801/

nodejs-github-bot · 2024-09-28T09:46:06Z

Landed in f5d454a

When creating an fast api the callback might use the receiver. In that case if the internal binding is destructured the method won't have access to the reciver and it will throw. Passing the receiver as second argument ensures the receiver is available. PR-URL: #54408 Reviewed-By: Stephen Belanger <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]>

When creating an fast api the callback might use the receiver. In that case if the internal binding is destructured the method won't have access to the reciver and it will throw. Passing the receiver as second argument ensures the receiver is available. PR-URL: nodejs#54408 Reviewed-By: Stephen Belanger <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]>

nodejs/node#54408

When creating an fast api the callback might use the receiver. In that case if the internal binding is destructured the method won't have access to the reciver and it will throw. Passing the receiver as second argument ensures the receiver is available. PR-URL: nodejs#54408 Reviewed-By: Stephen Belanger <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]>

nodejs/node#54408

* chore: bump node in DEPS to v22.11.0 * src: move evp stuff to ncrypto nodejs/node#54911 * crypto: add Date fields for validTo and validFrom nodejs/node#54159 * module: fix discrepancy between .ts and .js nodejs/node#54461 * esm: do not interpret "main" as a URL nodejs/node#55003 * src: modernize likely/unlikely hints nodejs/node#55155 * chore: update patch indices * crypto: add validFromDate and validToDate fields to X509Certificate nodejs/node#54159 * chore: fixup perfetto patch * fix: clang warning in simdjson * src: add receiver to fast api callback methods nodejs/node#54408 * chore: fixup revert patch * fixup! esm: do not interpret "main" as a URL * fixup! crypto: add Date fields for validTo and validFrom * fix: move ArrayBuffer test patch * src: fixup Error.stackTraceLimit during snapshot building nodejs/node#55121 * fix: bad rebase * chore: fixup amaro * chore: address feedback from review * src: revert filesystem::path changes nodejs/node#55015 --------- Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com> Co-authored-by: Shelley Vohr <[email protected]>

nodejs/node#54408

* chore: bump node in DEPS to v22.13.0 * chore: bump node in DEPS to v22.13.1 * src: move evp stuff to ncrypto nodejs/node#54911 * crypto: add Date fields for validTo and validFrom nodejs/node#54159 * module: fix discrepancy between .ts and .js nodejs/node#54461 * esm: do not interpret "main" as a URL nodejs/node#55003 * src: modernize likely/unlikely hints nodejs/node#55155 * chore: update patch indices * crypto: add validFromDate and validToDate fields to X509Certificate nodejs/node#54159 * chore: fixup perfetto patch * fix: clang warning in simdjson * src: add receiver to fast api callback methods nodejs/node#54408 * chore: fixup revert patch * fixup! esm: do not interpret "main" as a URL * fixup! crypto: add Date fields for validTo and validFrom * fix: move ArrayBuffer test patch * src: fixup Error.stackTraceLimit during snapshot building nodejs/node#55121 * fix: bad rebase * chore: fixup amaro * chore: address feedback from review * src: revert filesystem::path changes nodejs/node#55015 * chore: fixup GN build file * nodejs/node#55529 * nodejs/node#55798 * nodejs/node#55530 * module: simplify --inspect-brk handling nodejs/node#55679 * src: fix outdated js2c.cc references nodejs/node#56133 * crypto: include openssl/rand.h explicitly nodejs/node#55425 * build: use variable for crypto dep path nodejs/node#55928 * crypto: fix RSA_PKCS1_PADDING error message nodejs/node#55629 * build: use variable for simdutf path nodejs/node#56196 * test,crypto: make crypto tests work with BoringSSL nodejs/node#55491 * fix: suppress clang -Wdeprecated-declarations in libuv libuv/libuv#4486 * deps: update libuv to 1.49.1 nodejs/node#55114 * test: make test-node-output-v8-warning more flexible nodejs/node#55401 * [v22.x] Revert "v8: enable maglev on supported architectures" nodejs/node#54384 * fix: potential WIN32_LEAN_AND_MEAN redefinition c-ares/c-ares#869 * deps: update nghttp2 to 1.64.0 nodejs/node#55559 * src: provide workaround for container-overflow nodejs/node#55591 * build: use variable for simdutf path nodejs/node#56196 * chore: fixup patch indices * fixup! module: simplify --inspect-brk handling * lib: fix fs.readdir recursive async nodejs/node#56041 * lib: avoid excluding symlinks in recursive fs.readdir with filetypes nodejs/node#55714 This doesn't currently play well with ASAR - this should be fixed in a follow up * test: disable CJS permission test for config.main This has diverged as a result of our revert of src,lb: reducing C++ calls of esm legacy main resolve * fixup! lib: fix fs.readdir recursive async * deps: update libuv to 1.49.1 nodejs/node#55114 --------- Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com> Co-authored-by: Shelley Vohr <[email protected]>

The check was incorrectly altered in nodejs#54408, passing the bindings as argument for `this` instead of the path string. This check not working having no significant impact on Node.js as far as I can tell, this changes removes it altogether to avoid undesired overhead. Signed-off-by: Cynthia Rey <[email protected]>

The check was incorrectly altered in nodejs#54408, passing the bindings as argument for `this` instead of the path string. This check not working having no significant impact on Node.js as far as I can tell, this change removes it altogether to avoid undesired overhead. Signed-off-by: Cynthia Rey <[email protected]>

The check was incorrectly altered in nodejs#54408, passing the bindings as argument for `this` instead of the path string. Signed-off-by: Cynthia Rey <[email protected]>

nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Aug 16, 2024

Ceres6 mentioned this pull request Aug 16, 2024
Disallow destructuring internalbinding c #54023
Closed

Ceres6 marked this pull request as draft August 16, 2024 11:22

ronag requested a review from anonrig August 16, 2024 11:36

joyeecheung reviewed Sep 23, 2024
View reviewed changes

doc/contributing/adding-v8-fast-api.md Outdated Show resolvedHide resolved
doc/contributing/adding-v8-fast-api.mdShow resolvedHide resolved

Ceres6and others added 2 commits September 24, 2024 11:33

fixup! src: add receiver to fast api callback methods
b3e6133

Apply suggestions from code review
a5a555c
Co-authored-by: Joyee Cheung <[email protected]>

Ceres6 marked this pull request as ready for review September 24, 2024 09:43

Ceres6 requested a review from joyeecheung September 24, 2024 11:32

Qard approved these changes Sep 24, 2024
View reviewed changes

avivkeller reviewed Sep 24, 2024
View reviewed changes

doc/contributing/adding-v8-fast-api.md Outdated Show resolvedHide resolved

joyeecheung approved these changes Sep 24, 2024
View reviewed changes

anonrig approved these changes Sep 24, 2024
View reviewed changes

anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 24, 2024

github-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 24, 2024

Update doc/contributing/adding-v8-fast-api.md
41522d4
Co-authored-by: Aviv Keller <[email protected]>

santigimeno approved these changes Sep 24, 2024
View reviewed changes

aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. labels Sep 27, 2024

nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Sep 27, 2024

mcollina added commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. commit-queue Add this label to land a pull request using GitHub Actions. labels Sep 28, 2024

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 28, 2024

nodejs-github-bot merged commit f5d454a into nodejs:mainSep 28, 2024

aduh95 mentioned this pull request Oct 9, 2024
V22.10.0 proposal #55343
Merged

arthur-fontaine mentioned this pull request Nov 1, 2024
Node internal crash node::fs::InternalModuleStat starting from Node 22.10 httptoolkit/esm#3
Closed

marco-ippolito added the dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. label Nov 16, 2024

codebytere added a commit to electron/electron that referenced this pull request Nov 20, 2024
src: add receiver to fast api callback methods
4760b5e
nodejs/node#54408

codebytere added a commit to electron/electron that referenced this pull request Nov 21, 2024
src: add receiver to fast api callback methods
bd8a19a
nodejs/node#54408

codebytere added a commit to electron/electron that referenced this pull request Dec 3, 2024
src: add receiver to fast api callback methods
2ec1a0e
nodejs/node#54408

codebytere added a commit to electron/electron that referenced this pull request Dec 4, 2024
src: add receiver to fast api callback methods
1b8934a
nodejs/node#54408

codebytere added a commit to electron/electron that referenced this pull request Jan 22, 2025
src: add receiver to fast api callback methods
a80b815
nodejs/node#54408

codebytere added a commit to electron/electron that referenced this pull request Feb 3, 2025
src: add receiver to fast api callback methods
8537adc
nodejs/node#54408

codebytere added a commit to electron/electron that referenced this pull request Feb 10, 2025
src: add receiver to fast api callback methods
78b87bd
nodejs/node#54408

codebytere added a commit to electron/electron that referenced this pull request Feb 10, 2025
src: add receiver to fast api callback methods
1f595a2
nodejs/node#54408

codebytere added a commit to electron/electron that referenced this pull request Feb 14, 2025
src: add receiver to fast api callback methods
a25c4de
nodejs/node#54408

cyyynthia mentioned this pull request Apr 6, 2025
esm: remove check for trailing slash during resolve #57773
Open

Renegade334 mentioned this pull request Jul 13, 2025
src: fix broken fast callback signatures #59055
Closed

Renegade334 mentioned this pull request Aug 23, 2025
src: fix broken fast callback signatures #59600
Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

src: add receiver to fast api callback methods#54408

src: add receiver to fast api callback methods #54408

Uh oh!

Ceres6 commented Aug 16, 2024

Uh oh!

nodejs-github-bot commented Aug 16, 2024

Uh oh!

Ceres6 commented Aug 16, 2024

Uh oh!

codecovbot commented Aug 16, 2024•
edited
Loading

Uh oh!

joyeecheung left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nodejs-github-bot commented Sep 24, 2024

Uh oh!

Ceres6 commented Sep 25, 2024

Uh oh!

nodejs-github-bot commented Sep 27, 2024

Uh oh!

nodejs-github-bot commented Sep 27, 2024

Uh oh!

nodejs-github-bot commented Sep 28, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

Uh oh!

src: add receiver to fast api callback methods#54408

src: add receiver to fast api callback methods #54408

Uh oh!

Conversation

Ceres6 commented Aug 16, 2024

Uh oh!

nodejs-github-bot commented Aug 16, 2024

Uh oh!

Ceres6 commented Aug 16, 2024

Uh oh!

codecovbot commented Aug 16, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

joyeecheung left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nodejs-github-bot commented Sep 24, 2024

Uh oh!

Ceres6 commented Sep 25, 2024

Uh oh!

nodejs-github-bot commented Sep 27, 2024

Uh oh!

nodejs-github-bot commented Sep 27, 2024

Uh oh!

nodejs-github-bot commented Sep 28, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

codecovbot commented Aug 16, 2024•
edited
Loading