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

fs: refactor to use optional chaining #36524

Closed
wants to merge 1 commit into from

Conversation

Lxxyx
Copy link
Member

@Lxxyx Lxxyx commented Dec 15, 2020

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Dec 15, 2020
Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

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

object?.method?.() is equivalent to if(object && object.method) object.method(), not if(object) object.method(). I think we should not add extra checks to avoid behaviour changes, and it might impact perf.

lib/fs.js Outdated Show resolved Hide resolved
lib/fs.js Outdated Show resolved Hide resolved
lib/fs.js Outdated Show resolved Hide resolved
lib/fs.js Outdated Show resolved Hide resolved
lib/internal/fs/rimraf.js Outdated Show resolved Hide resolved
lib/internal/fs/watchers.js Outdated Show resolved Hide resolved
@Lxxyx Lxxyx force-pushed the fs-refactor-use-optional-chaining branch from f9d9035 to 8b71da7 Compare December 15, 2020 13:35
@Lxxyx Lxxyx requested a review from aduh95 December 15, 2020 13:35
@Lxxyx
Copy link
Member Author

Lxxyx commented Dec 15, 2020

object?.method?.() is equivalent to if(object && object.method) object.method(), not if(object) object.method(). I think we should not add extra checks to avoid behaviour changes, and it might impact perf.

Yes, thanks for the guidance.

@jasnell
Copy link
Member

jasnell commented Dec 15, 2020

Benchmark run (link may not yet be active): https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/760/

Nevermind.. looks like @mscdex beat me to it. There was a benchmark job running already but I hadn't noticed that it was for the same PR.

https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/759/

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

Performance benchmark results look good! Happy to see us start to make these changes. The one downside is that it'll make backporting a bit more difficult

@jasnell jasnell added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 15, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 15, 2020
@nodejs-github-bot
Copy link
Collaborator

@Lxxyx
Copy link
Member Author

Lxxyx commented Dec 16, 2020

Performance benchmark results look good! Happy to see us start to make these changes. The one downside is that it'll make backporting a bit more difficult

Yes, and this will make our code easier to read. And I have two questions and would like to know what you think.

  1. I noticed that there are many modules that can be converted to optional chaining.Should we modify multiple modules in a single Pull Request?

  2. You mentioned that this make backporting a bit more difficult, should we stop such work for now?

@jasnell
Copy link
Member

jasnell commented Dec 16, 2020

Incremental changes are best. And no, backporting shouldn't hold this up.

@Lxxyx
Copy link
Member Author

Lxxyx commented Dec 16, 2020

Incremental changes are best. And no, backporting shouldn't hold this up.

I know what to do now, thanks for your guidance.

@aduh95 aduh95 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 17, 2020
@lpinca
Copy link
Member

lpinca commented Dec 17, 2020

Yes, and this will make our code easier to read.

I disagree :)

@Lxxyx
Copy link
Member Author

Lxxyx commented Dec 18, 2020

Yes, and this will make our code easier to read.

I disagree :)

When there is a difference of opinion on code style, I think we should look at what the community thinks.

TypeScript Pull Request (Add support for Optional Chaining):
microsoft/TypeScript#33294

image

Reference: MDN

This results in shorter and simpler expressions when accessing chained properties when the possibility exists that a reference may be missing.

Reference: V8: Optional chaining

// Error prone-version, could throw.
const nameLength = db.user.name.length;

// Less error-prone, but harder to read.
let nameLength;
if (db && db.user && db.user.name)
  nameLength = db.user.name.length;

// The above can also be expressed using the ternary operator, which doesn’t exactly help readability
const nameLength =
  (db
    ? (db.user
      ? (db.user.name
        ? db.user.name.length
        : undefined)
      : undefined)
    : undefined);

// Still checks for errors and is much more readable.
const nameLength = db?.user?.name?.length;

targos pushed a commit that referenced this pull request Dec 21, 2020
PR-URL: #36524
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@targos
Copy link
Member

targos commented Dec 21, 2020

Landed in 54e763d

@targos targos closed this Dec 21, 2020
targos pushed a commit that referenced this pull request Dec 21, 2020
PR-URL: #36524
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants