lib: prefer optional chaining

[avivkeller]

No description provided.

[nodejs-github-bot]

Review requested:

• @nodejs/crypto
• @nodejs/http
• @nodejs/loaders
• @nodejs/net
• @nodejs/startup
• @nodejs/streams

[lpinca]

Why? I won't block, but this is a cosmetic change that does not add any value in my opinion. It only adds noise to git blame.

[avivkeller]

IIRC optional chaining is slightly faster.

Quick and dirty JSBench shows a ~0.3-1% improvement in ops/sec: https://jsben.ch/znvKH

[lpinca]

The performance is the same:

const Benchmark = require('benchmark');

const suite = new Benchmark.Suite();

const testBad = {};

suite.add('Optional chaining', function () {
  let wow;

  if (testBad?.id === '123') {
    wow = 'ok';
  }

  return wow;
});

suite.add('Without optional chaining', function () {
  let wow;

  if (testBad && testBad.id && testBad.id === '123') {
    wow = 'ok';
  }

  return wow;
});

suite.on('cycle', function (event) {
  console.log(event.target.toString());
});

suite.on('complete', function () {
  console.log('Fastest is ' + this.filter('fastest').map('name'));
});

suite.run({ async: true });

$ node index.js 
Optional chaining x 103,663,582 ops/sec ±6.25% (75 runs sampled)
Without optional chaining x 106,320,156 ops/sec ±5.14% (76 runs sampled)
Fastest is Without optional chaining,Optional chaining

[avivkeller]

ahh interesting. In that case, then yes, this is purely cosmetic, but I still think that using optional chaining is more concise and readable than without, but if it adds too much noise, it's not a needed change, however I'd love to see some other opinions.

[panva]

I won't block but i'd say this ought to be accompanied by a linter enforcement. And that's when this will be debated ;)

[codecov]

Codecov Report

Attention: Patch coverage is 93.33333% with 5 lines in your changes missing coverage. Please review.

Project coverage is 88.24%. Comparing base (8b8fc53) to head (dbf1046).
Report is 445 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/internal/modules/esm/translators.js	0.00%	2 Missing ⚠️
lib/_tls_wrap.js	50.00%	0 Missing and 1 partial ⚠️
lib/assert.js	66.66%	1 Missing ⚠️
lib/net.js	66.66%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #55045   +/-   ##
=======================================
  Coverage   88.23%   88.24%           
=======================================
  Files         652      652           
  Lines      183855   183896   +41     
  Branches    35856    35856           
=======================================
+ Hits       162227   162274   +47     
+ Misses      14909    14908    -1     
+ Partials     6719     6714    -5     

Files with missing lines	Coverage Δ
lib/_http_agent.js	98.19% <100.00%> (ø)
lib/_http_client.js	97.96% <100.00%> (ø)
lib/_http_common.js	100.00% <100.00%> (ø)
lib/_http_incoming.js	99.33% <100.00%> (ø)
lib/_http_outgoing.js	95.05% <100.00%> (ø)
lib/_http_server.js	96.74% <100.00%> (+0.08%)	⬆️
lib/child_process.js	97.74% <100.00%> (-0.01%)	⬇️
lib/dgram.js	97.31% <100.00%> (ø)
lib/internal/bootstrap/switches/is_main_thread.js	95.91% <100.00%> (ø)
lib/internal/child_process.js	94.72% <100.00%> (ø)
... and 24 more

... and 50 files with indirect coverage changes

[mcollina]

lgtm

[nodejs-github-bot]

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

[lpinca]

Going forward, has the project's stance on cosmetic changes changed? We've rejected dozens of patches like this in the past. I think that approving some while rejecting others is disrespectful to contributors who have theirs rejected.

[anonrig]

Going forward, has the project's stance on cosmetic changes changed?

Once I think about it, I think I agree with @lpinca. This needs to come with a eslint rule.

[avivkeller]

@anonrig I added a prefer-optional-chaining lint rule. Are you still blocking?

[mertcanaltin]

lgtm

[nodejs-github-bot]

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

[atlowChemi]

@anonrig Would you mind re-reviewing or dismissing the request changes, as your concerns were addressed?

[nodejs-github-bot]

Landed in 574f2dd