src: accept single argument in getProxyDetails #30858

BridgeAR · 2019-12-08T20:47:45Z

This makes sure this function stays backwards compatible in case
it's accessed through the binding directly.

Checklist

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

This makes sure this function stays backwards compatible in case it's accessed through the binding directly. Refs: nodejs#29947 (comment)

nodejs-github-bot · 2019-12-08T23:42:25Z

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

Trott · 2019-12-08T23:43:46Z

CITGM to confirm that this fixes the issue found there: https://ci.nodejs.org/view/All/job/citgm-smoker/2116/

Trott · 2019-12-08T23:45:45Z

If CI is green and CITGM is what we would expect, I would be inclined to fast-track. Collaborators, 👍here to approve fast-tracking, or leave a comment to stop it.

nodejs-github-bot · 2019-12-09T09:52:18Z

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

bnoordhuis · 2019-12-09T10:25:18Z

src/node_util.cc

  Local<Proxy> proxy = args[0].As<Proxy>();

-  if (args[1]->IsTrue()) {
+  // The length check keeps this function backwards compatible.


I think it would be good to name-and-shame here, otherwise the comment is going to befuddle future maintainers. Things at the binding layer don't normally have to worry about backwards compatibility.

I changed it to a TODO statement. PTAL.

bnoordhuis · 2019-12-09T11:00:29Z

src/node_util.cc

  Local<Proxy> proxy = args[0].As<Proxy>();

-  if (args[1]->IsTrue()) {
+  // TODO(BridgeAR): Remove the length check as soon as we prohibit access to
+  // the util binding layer. It's accessed in the wild and some code would break


I'd be more explicit than "some code". We know of esm. Are there other packages (that are popular)?

addaleax · 2019-12-09T14:01:43Z

src/node_util.cc

+  // TODO(BridgeAR): Remove the length check as soon as we prohibit access to
+  // the util binding layer. It's accessed in the wild and `esm` would break in
+  // case the check is removed.
+  if (args.Length() == 1 || args[1]->IsTrue()) {


The length check here doesn’t change behaviour and could safely be dropped, so you can apply the TODO comment right now if you want.

You mean if I write if (!args[1]->IsFalse()) { ...old behavior... } else { ...new behavior... }?

Yes, something like that … feel free to ignore though :)

joyeecheung · 2019-12-10T15:17:56Z

src/node_util.cc

-  if (args[1]->IsTrue()) {
+  // TODO(BridgeAR): Remove the length check as soon as we prohibit access to
+  // the util binding layer. It's accessed in the wild and `esm` would break in
+  // case the check is removed.


Is there an issue opened in esm? (BTW it is somewhat ambiguous what esm means here..unless it's explicitly pointed out that this is about the npm package)

nodejs-github-bot · 2019-12-11T12:48:07Z

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

This makes sure this function stays backwards compatible in case it's accessed through the binding directly. Refs: #29947 (comment) PR-URL: #30858 Refs: #30767 Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Anna Henningsen <anna@addaleax.net>

Trott · 2019-12-11T23:49:32Z

Landed in fb14ed4

This makes sure this function stays backwards compatible in case it's accessed through the binding directly. Refs: #29947 (comment) PR-URL: #30858 Refs: #30767 Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Anna Henningsen <anna@addaleax.net>

targos · 2020-01-14T11:59:13Z

Depends on #30767 to land on v12.x-staging

This makes sure this function stays backwards compatible in case it's accessed through the binding directly. Refs: nodejs#29947 (comment) PR-URL: nodejs#30858 Refs: nodejs#30767 Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Anna Henningsen <anna@addaleax.net>

This makes sure this function stays backwards compatible in case it's accessed through the binding directly. Refs: #29947 (comment) Backport-PR-URL: #31431 PR-URL: #30858 Refs: #30767 Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Anna Henningsen <anna@addaleax.net>

src: accept single argument in getProxyDetails

578f0b0

This makes sure this function stays backwards compatible in case it's accessed through the binding directly. Refs: nodejs#29947 (comment)

nodejs-github-bot added c++ util labels Dec 8, 2019

richardlau approved these changes Dec 8, 2019

View changes

Trott approved these changes Dec 8, 2019

View changes

Trott added the fast-track label Dec 8, 2019

devnexen approved these changes Dec 9, 2019

View changes

ZYSzys approved these changes Dec 9, 2019

View changes

targos approved these changes Dec 9, 2019

View changes

bnoordhuis reviewed Dec 9, 2019

View changes

fixup: change comment to TODO

e42bfc6

bnoordhuis approved these changes Dec 9, 2019

View changes

fixup: mention esm explictly

aa2f0bf

addaleax approved these changes Dec 9, 2019

View changes

richardlau mentioned this pull request Dec 10, 2019

SEGFAULT in CITGM #30879

Closed

joyeecheung reviewed Dec 10, 2019

View changes

Trott mentioned this pull request Dec 11, 2019

deps: upgrade npm to 6.13.3 #30873

Closed

Trott closed this Dec 11, 2019

MylesBorins mentioned this pull request Dec 13, 2019

v13.4.0 proposal #30937

Merged

targos added the backport-blocked-v12.x label Jan 14, 2020

BridgeAR deleted the fix-get-proxy-details branch Jan 20, 2020

BridgeAR added backport-open-v12.x backport-requested-v12.x and removed backport-blocked-v12.x labels Jan 22, 2020

BridgeAR mentioned this pull request Jan 22, 2020

[v12.x] multiple util and assert backports #31431

Closed

4 tasks

MylesBorins added backported-to-v12.x and removed backport-open-v12.x backport-requested-v12.x labels Jan 30, 2020

targos mentioned this pull request Jan 30, 2020

v12.15.0 release proposal #31368

Closed

MylesBorins mentioned this pull request Feb 8, 2020

v12.16.0 proposal #31691

Merged

nodejs / node Public

src: accept single argument in getProxyDetails #30858

src: accept single argument in getProxyDetails #30858

BridgeAR commented Dec 8, 2019

nodejs-github-bot commented Dec 8, 2019

Trott commented Dec 8, 2019

Trott commented Dec 8, 2019

nodejs-github-bot commented Dec 9, 2019

bnoordhuis Dec 9, 2019

BridgeAR Dec 9, 2019

bnoordhuis Dec 9, 2019

addaleax Dec 9, 2019

BridgeAR Dec 9, 2019

addaleax Dec 10, 2019

joyeecheung Dec 10, 2019

nodejs-github-bot commented Dec 11, 2019

Trott commented Dec 11, 2019

targos commented Jan 14, 2020

nodejs / node Public

src: accept single argument in getProxyDetails #30858

src: accept single argument in getProxyDetails #30858

Conversation

BridgeAR commented Dec 8, 2019

Checklist

nodejs-github-bot commented Dec 8, 2019

Trott commented Dec 8, 2019

Trott commented Dec 8, 2019

nodejs-github-bot commented Dec 9, 2019

bnoordhuis Dec 9, 2019

Choose a reason for hiding this comment

BridgeAR Dec 9, 2019

Choose a reason for hiding this comment

bnoordhuis Dec 9, 2019

Choose a reason for hiding this comment

addaleax Dec 9, 2019

Choose a reason for hiding this comment

BridgeAR Dec 9, 2019

Choose a reason for hiding this comment

addaleax Dec 10, 2019

Choose a reason for hiding this comment

joyeecheung Dec 10, 2019

Choose a reason for hiding this comment

nodejs-github-bot commented Dec 11, 2019

Trott commented Dec 11, 2019

targos commented Jan 14, 2020