Skip to content

lib: refactor to use optional chaining#38697

Closed
pd4d10 wants to merge 1 commit intonodejs:mainfrom
pd4d10:patch-lib-optional
Closed

lib: refactor to use optional chaining#38697
pd4d10 wants to merge 1 commit intonodejs:mainfrom
pd4d10:patch-lib-optional

Conversation

@pd4d10
Copy link
Contributor

@pd4d10 pd4d10 commented May 16, 2021 •
edited
Loading

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

@pd4d10
lib: refactor to use optional chaining
d963689 
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
@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.

aduh95
aduh95 reviewed May 16, 2021
View reviewed changes
lib/internal/child_process.js

get fd() {
return this.#channel ? this.#channel.fd : undefined;
return this.#channel?.fd ?? 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
return this.#channel?.fd ?? undefined;
return this.#channel?.fd;

lib/internal/http2/compat.js
return false;
const request = stream[kRequest];
return request ? request.readable : stream.readable;
return request?.readable ?? stream.readable;
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
return request?.readable ?? stream.readable;
return stream[kRequest]?.readable ?? stream.readable;

lib/internal/http2/core.js
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 •
edited
Loading

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];

lib/_http_outgoing.js

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

@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 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.

Mifrill
Mifrill reviewed Oct 26, 2023
View reviewed changes
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
@github-actions
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.

@github-actions
Copy link
Contributor

github-actions bot commented Jun 12, 2024

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

Reviewers

@aduh95 aduh95 aduh95 left review comments

@jasnell jasnell jasnell approved these changes

+1 more reviewer

@Mifrill Mifrill Mifrill left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

stalled Issues and PRs that are stalled.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@pd4d10 @aduh95 @jasnell @Mifrill