src: accept single argument in getProxyDetails by BridgeAR · Pull Request #30858 · nodejs/node

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:22Z

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 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:39Z

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:53Z

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++ Issues and PRs that require attention from people who are familiar with C++. util Issues and PRs related to the built-in util module. labels Dec 8, 2019

richardlau approved these changes Dec 8, 2019

View reviewed changes

Trott approved these changes Dec 8, 2019

View reviewed changes

Trott added the fast-track PRs that do not need to wait for 48 hours to land. label Dec 8, 2019

devnexen approved these changes Dec 9, 2019

View reviewed changes

ZYSzys approved these changes Dec 9, 2019

View reviewed changes

targos approved these changes Dec 9, 2019

View reviewed changes

bnoordhuis reviewed Dec 9, 2019

View reviewed changes

fixup: change comment to TODO

e42bfc6

bnoordhuis approved these changes Dec 9, 2019

View reviewed changes

fixup: mention esm explictly

aa2f0bf

addaleax approved these changes Dec 9, 2019

View reviewed changes

richardlau mentioned this pull request Dec 10, 2019

SEGFAULT in CITGM #30879

Closed

joyeecheung reviewed Dec 10, 2019

View reviewed 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 January 20, 2020 12:07

BridgeAR added backport-open-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 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

Uh oh!

Conversation

BridgeAR commented Dec 8, 2019

Checklist

Uh oh!

nodejs-github-bot commented Dec 8, 2019

Uh oh!

Trott commented Dec 8, 2019

Uh oh!

Trott commented Dec 8, 2019

Uh oh!

nodejs-github-bot commented Dec 9, 2019

Uh oh!

bnoordhuis Dec 9, 2019

Choose a reason for hiding this comment

Uh oh!

BridgeAR Dec 9, 2019

Choose a reason for hiding this comment

Uh oh!

bnoordhuis Dec 9, 2019

Choose a reason for hiding this comment

Uh oh!

addaleax Dec 9, 2019

Choose a reason for hiding this comment

Uh oh!

BridgeAR Dec 9, 2019

Choose a reason for hiding this comment

Uh oh!

addaleax Dec 10, 2019

Choose a reason for hiding this comment

Uh oh!

joyeecheung Dec 10, 2019

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Dec 11, 2019

Uh oh!

Trott commented Dec 11, 2019

Uh oh!

targos commented Jan 14, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants