Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

src: accept single argument in getProxyDetails #30858

Closed
wants to merge 3 commits into from

Conversation

BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented Dec 8, 2019

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

Refs: #29947 (comment)
Refs: #30767

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 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
@nodejs-github-bot
Copy link
Collaborator

@Trott
Copy link
Member

Trott commented Dec 8, 2019

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

@Trott
Copy link
Member

Trott commented Dec 8, 2019

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.

@Trott Trott added the fast-track PRs that do not need to wait for 48 hours to land. label Dec 8, 2019
@nodejs-github-bot
Copy link
Collaborator

src/node_util.cc Outdated
Local<Proxy> proxy = args[0].As<Proxy>();

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed it to a TODO statement. PTAL.

src/node_util.cc Outdated
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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// 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()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@richardlau richardlau mentioned this pull request Dec 10, 2019
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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Collaborator

Trott pushed a commit that referenced this pull request Dec 11, 2019
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
Copy link
Member

Trott commented Dec 11, 2019

Landed in fb14ed4

@Trott Trott closed this Dec 11, 2019
MylesBorins pushed a commit that referenced this pull request Dec 13, 2019
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>
@MylesBorins MylesBorins mentioned this pull request Dec 13, 2019
@targos
Copy link
Member

targos commented Jan 14, 2020

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

@BridgeAR BridgeAR deleted the fix-get-proxy-details branch January 20, 2020 12:07
BridgeAR added a commit to BridgeAR/node that referenced this pull request Jan 22, 2020
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>
MylesBorins pushed a commit that referenced this pull request Jan 30, 2020
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>
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
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>
@MylesBorins MylesBorins mentioned this pull request Feb 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. fast-track PRs that do not need to wait for 48 hours to land. util Issues and PRs related to the built-in util module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.