fs: refactor to use optional chaining

[Lxxyx]

Checklist

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

[aduh95]

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.

[Lxxyx]

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]

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/

[jasnell]

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

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/34964/

[Lxxyx]

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]

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

[Lxxyx]

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

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

[lpinca]

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

I disagree :)

[Lxxyx]

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

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]

Landed in 54e763d