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

lib: refactor to use optional chaining #38697

Closed
wants to merge 1 commit into from

Conversation

pd4d10
Copy link
Contributor

@pd4d10 pd4d10 commented May 16, 2021

This PR migrates expressions such as a ? a.b : c to a?.b ?? c

Codemod script:
https://github.com/pd4d10/nodejs-codemod/blob/main/src/optional-chaining.ts

Also see:
#38609

@aduh95
Copy link
Contributor

aduh95 commented May 16, 2021

I'm not sure we want to do that after #38245...
In any case this should be split into several PRs, it's less of a burden to review smaller PRs and we benchmark it more efficiently.

@@ -561,7 +561,7 @@ class Control extends EventEmitter {
}

get fd() {
return this.#channel ? this.#channel.fd : undefined;
return this.#channel?.fd ?? undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return this.#channel?.fd ?? undefined;
return this.#channel?.fd;

@@ -221,7 +221,7 @@ const proxySocketHandler = {
if (stream.destroyed)
return false;
const request = stream[kRequest];
return request ? request.readable : stream.readable;
return request?.readable ?? stream.readable;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return request?.readable ?? stream.readable;
return stream[kRequest]?.readable ?? stream.readable;

@@ -199,7 +199,7 @@ function debugStream(id, sessionType, message, ...args) {

function debugStreamObj(stream, message, ...args) {
const session = stream[kSession];
const type = session ? session[kType] : undefined;
const type = session?.kType ?? undefined;
Copy link
Contributor

@aduh95 aduh95 May 16, 2021

Choose a reason for hiding this comment

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

Suggested change
const type = session?.kType ?? undefined;
const type = stream[kSession]?.[kType];

@@ -709,7 +709,7 @@ OutgoingMessage.prototype.write = function write(chunk, encoding, callback) {
};

function onError(msg, err, callback) {
const triggerAsyncId = msg.socket ? msg.socket[async_id_symbol] : undefined;
const triggerAsyncId = msg.socket?.[async_id_symbol] ?? undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const triggerAsyncId = msg.socket?.[async_id_symbol] ?? undefined;
const triggerAsyncId = msg.socket?.[async_id_symbol];

@pd4d10
Copy link
Contributor Author

pd4d10 commented May 16, 2021

I'm not sure we want to do that after #38245...

OK. Given this information, I guess we should hold it until the performance issues solved.

Copy link

@Mifrill Mifrill left a comment

Choose a reason for hiding this comment

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

Those changes look great, and I see the potential performance issue was also taken into consideration, that's awesome, however would like to raise a hand and ask if the chaining is slower than the ternary operator.

📝 CC @ronag #50337 (comment)

@aduh95 aduh95 added the stalled Issues and PRs that are stalled. label May 5, 2024
Copy link
Contributor

github-actions bot commented May 5, 2024

This issue/PR was marked as stalled, it will be automatically closed in 30 days. If it should remain open, please leave a comment explaining why it should remain open.

Copy link
Contributor

Closing this because it has stalled. Feel free to reopen if this issue/PR is still relevant, or to ping the collaborator who labelled it stalled if you have any questions.

@github-actions github-actions bot closed this Jun 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stalled Issues and PRs that are stalled.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants