GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — lib: refactor to use optional chaining by pd4d10 · Pull Request #38697 · nodejs/node

Conversation

@pd4d10

@pd4d10 pd4d10 commented May 16, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bricss and Mifrill reacted with eyes emoji
All reactions
  • 👀 2 reactions
@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

aduh95 commented May 16, 2021

Copy link
Copy Markdown
Contributor

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.
Mifrill reacted with eyes emoji
All reactions
  • 👀 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
aduh95
aduh95 reviewed May 16, 2021
View reviewed changes
Comment thread lib/internal/child_process.js

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

@aduh95 aduh95 May 16, 2021

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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;

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread lib/internal/http2/compat.js
return false;
const request = stream[kRequest];
return request ? request.readable : stream.readable;
return request?.readable ?? stream.readable;

@aduh95 aduh95 May 16, 2021

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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;

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread 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;

@aduh95 aduh95 May 16, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread 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;

@aduh95 aduh95 May 16, 2021

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@pd4d10

pd4d10 commented May 16, 2021

Copy link
Copy Markdown
Contributor Author

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 reacted with eyes emoji
All reactions
  • 👀 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
jasnell
jasnell approved these changes May 17, 2021
View reviewed changes
@Trott
Trott force-pushed the main branch from 2d76238 to ca3ed36 Compare November 12, 2022 01:49
Mifrill
Mifrill reviewed Oct 26, 2023
View reviewed changes

@Mifrill Mifrill left a comment

Copy link
Copy Markdown

There was a problem hiding this 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)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@aduh95 aduh95 added the stalled Issues and PRs manually marked as stalled and scheduled for automatic closure. label May 5, 2024
@github-actions

github-actions Bot commented May 5, 2024

Copy link
Copy Markdown
Contributor

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@github-actions

github-actions Bot commented Jun 12, 2024

Copy link
Copy Markdown
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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

stalled Issues and PRs manually marked as stalled and scheduled for automatic closure.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@pd4d10 @aduh95 @jasnell @Mifrill
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.