src,test,tools: forbid usage of v8::Persistent #31018

addaleax · 2019-12-18T14:34:40Z

src,test: use v8::Global instead of v8::Persistent

This is in preparation for a lint rule forbidding usage of v8::Persistent.

doc: avoid using v8::Persistent in addon docs

Use v8::Global where possible. For examples where it applies,
also clean up the code and make the code multi-threading-ready,
for those where that isn’t easily possible, add a warning about that.

tools,src: forbid usage of v8::Persistent

v8::Persistent comes with the surprising catch that it requires manual cleanup. v8::Global doesn’t, making it easier to use, and additionally provides move semantics. New code should always use v8::Global.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines


        src,test: use v8::Global instead of v8::Persistent

This is in preparation for a lint rule forbidding usage of `v8::Persistent`.


        fixup! src,test: use v8::Global instead of v8::Persistent


        tools,src: forbid usage of v8::Persistent

`v8::Persistent` comes with the surprising catch that it requires manual cleanup. `v8::Global` doesn’t, making it easier to use, and additionally provides move semantics. New code should always use `v8::Global`.


        doc: avoid using v8::Persistent in addon docs

Use `v8::Global` where possible. For examples where it applies, also clean up the code and make the code multi-threading-ready, for those where that isn’t easily possible, add a warning about that.

addaleax · 2019-12-18T15:50:23Z

@cjihrig @richardlau Can you take another look? I had to push some changes to the addon docs, as those weren’t linted when running make lint locally.

cjihrig · 2019-12-18T16:54:40Z

Still LGTM

nodejs-github-bot · 2019-12-18T18:12:36Z

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

addaleax · 2019-12-19T14:51:02Z

Any ideas why the CI comment here is marked outdated or who hid it? Afaict, this should be ready with a yellow CI.

richardlau · 2019-12-19T15:10:25Z

Any ideas why the CI comment here is marked outdated or who hid it? Afaict, this should be ready with a yellow CI.

Sorry, that was me. I misread GitHub and thought that 335d450 was newer than 0c0d530 but having checked again I see I was mistaken 🙇 and have unhidden the comment now.

Qard

LGTM, with a minor nit, but I don't want it to block landing this--just something to think about.

doc/api/addons.md

addaleax · 2019-12-24T13:38:44Z

Landed in db109e8...e23bf8f


        src,test: use v8::Global instead of v8::Persistent

This is in preparation for a lint rule forbidding usage of `v8::Persistent`. PR-URL: #31018 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>


        doc: avoid using v8::Persistent in addon docs

Use `v8::Global` where possible. For examples where it applies, also clean up the code and make the code multi-threading-ready, for those where that isn’t easily possible, add a warning about that. PR-URL: #31018 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>


        tools,src: forbid usage of v8::Persistent

`v8::Persistent` comes with the surprising catch that it requires manual cleanup. `v8::Global` doesn’t, making it easier to use, and additionally provides move semantics. New code should always use `v8::Global`. PR-URL: #31018 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>


        src,test: use v8::Global instead of v8::Persistent

This is in preparation for a lint rule forbidding usage of `v8::Persistent`. PR-URL: #31018 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>


        doc: avoid using v8::Persistent in addon docs

Use `v8::Global` where possible. For examples where it applies, also clean up the code and make the code multi-threading-ready, for those where that isn’t easily possible, add a warning about that. PR-URL: #31018 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>


        tools,src: forbid usage of v8::Persistent

`v8::Persistent` comes with the surprising catch that it requires manual cleanup. `v8::Global` doesn’t, making it easier to use, and additionally provides move semantics. New code should always use `v8::Global`. PR-URL: #31018 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>


        src,test: use v8::Global instead of v8::Persistent

This is in preparation for a lint rule forbidding usage of `v8::Persistent`. PR-URL: #31018 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>


        doc: avoid using v8::Persistent in addon docs

Use `v8::Global` where possible. For examples where it applies, also clean up the code and make the code multi-threading-ready, for those where that isn’t easily possible, add a warning about that. PR-URL: #31018 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>


        tools,src: forbid usage of v8::Persistent

`v8::Persistent` comes with the surprising catch that it requires manual cleanup. `v8::Global` doesn’t, making it easier to use, and additionally provides move semantics. New code should always use `v8::Global`. PR-URL: #31018 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>


        src,test: use v8::Global instead of v8::Persistent

This is in preparation for a lint rule forbidding usage of `v8::Persistent`. PR-URL: #31018 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>


        doc: avoid using v8::Persistent in addon docs

Use `v8::Global` where possible. For examples where it applies, also clean up the code and make the code multi-threading-ready, for those where that isn’t easily possible, add a warning about that. PR-URL: #31018 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>


        tools,src: forbid usage of v8::Persistent

`v8::Persistent` comes with the surprising catch that it requires manual cleanup. `v8::Global` doesn’t, making it easier to use, and additionally provides move semantics. New code should always use `v8::Global`. PR-URL: #31018 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>


        src,test: use v8::Global instead of v8::Persistent

This is in preparation for a lint rule forbidding usage of `v8::Persistent`. PR-URL: nodejs#31018 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>


        doc: avoid using v8::Persistent in addon docs

Use `v8::Global` where possible. For examples where it applies, also clean up the code and make the code multi-threading-ready, for those where that isn’t easily possible, add a warning about that. PR-URL: nodejs#31018 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>


        tools,src: forbid usage of v8::Persistent

`v8::Persistent` comes with the surprising catch that it requires manual cleanup. `v8::Global` doesn’t, making it easier to use, and additionally provides move semantics. New code should always use `v8::Global`. PR-URL: nodejs#31018 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>

nodejs-github-bot added C++ tools labels Dec 18, 2019

addaleax mentioned this pull request Dec 18, 2019

async_hooks: add executionAsyncResource #30959

Closed

4 of 4 tasks complete

cjihrig approved these changes Dec 18, 2019

View changes

richardlau approved these changes Dec 18, 2019

View changes

addaleax added the work in progress (WIP) label Dec 18, 2019

addaleax added 4 commits Dec 18, 2019

src,test: use v8::Global instead of v8::Persistent

Verified

This commit was signed with a verified signature.

addaleax Anna Henningsen

GPG key ID: 9C63F3A6CD2AD8F9 Learn about signing commits

d89dff2

This is in preparation for a lint rule forbidding usage of `v8::Persistent`.

fixup! src,test: use v8::Global instead of v8::Persistent

Verified

This commit was signed with a verified signature.

addaleax Anna Henningsen

GPG key ID: 9C63F3A6CD2AD8F9 Learn about signing commits

f76f6a8

addaleax removed the work in progress (WIP) label Dec 18, 2019

addaleax force-pushed the addaleax:no-persistent branch to 0c0d530 Dec 18, 2019

richardlau approved these changes Dec 18, 2019

View changes

jasnell approved these changes Dec 18, 2019

View changes

devnexen approved these changes Dec 18, 2019

View changes

addaleax added the author ready label Dec 18, 2019

Trott approved these changes Dec 18, 2019

View changes

devsnek approved these changes Dec 18, 2019

View changes

joyeecheung approved these changes Dec 19, 2019

View changes

bnoordhuis approved these changes Dec 19, 2019

View changes

Qard approved these changes Dec 22, 2019

View changes

doc/api/addons.md Show resolved Hide resolved

addaleax closed this Dec 24, 2019

addaleax deleted the addaleax:no-persistent branch Dec 24, 2019

BridgeAR mentioned this pull request Jan 7, 2020

v13.6.0 proposal #31238

Merged

targos mentioned this pull request Jan 15, 2020

v12.15.0 release proposal #31368

Closed

MylesBorins mentioned this pull request Feb 8, 2020

v12.16.0 proposal #31691

Merged

Mar	APR	May
	23
2019	2020	2021

nodejs / node

src,test,tools: forbid usage of v8::Persistent #31018

src,test,tools: forbid usage of v8::Persistent #31018

addaleax commented Dec 18, 2019 •

edited

This comment has been minimized.

addaleax commented Dec 18, 2019

This comment has been minimized.

cjihrig commented Dec 18, 2019

This comment has been minimized.

nodejs-github-bot commented Dec 18, 2019

This comment has been minimized.

addaleax commented Dec 19, 2019

This comment has been minimized.

richardlau commented Dec 19, 2019

Qard left a comment

This comment has been minimized.

addaleax commented Dec 24, 2019

nodejs / node

Join GitHub today

src,test,tools: forbid usage of v8::Persistent #31018

src,test,tools: forbid usage of v8::Persistent #31018

Conversation

addaleax commented Dec 18, 2019 • edited

src,test: use v8::Global instead of v8::Persistent

doc: avoid using v8::Persistent in addon docs

tools,src: forbid usage of v8::Persistent

Checklist

This comment has been minimized.

addaleax commented Dec 18, 2019

This comment has been minimized.

cjihrig commented Dec 18, 2019

This comment has been minimized.

nodejs-github-bot commented Dec 18, 2019

This comment has been minimized.

addaleax commented Dec 19, 2019

This comment has been minimized.

richardlau commented Dec 19, 2019

Qard left a comment

This comment has been minimized.

addaleax commented Dec 24, 2019

addaleax commented Dec 18, 2019 •

edited