chore: no-floating-promises

[naseemkullah]

Signed-off-by: Naseem [email protected]

Which problem is this PR solving?

• solves eslint config: "@typescript-eslint/no-floating-promises": 2 #1492

Short description of the changes

• added https://palantir.github.io/tslint/rules/no-floating-promises/ and configured accordingly

It was mostly tests that had floating promises (if there's been any test flakiness it could possibly be attributed to that), but there were a few floating promises in non-test code.

[naseemkullah]

You'll want to ignore white spaces when reviewing this one.

[obecny]

I guess in many cases it would be enough to add catch instead of changing this from promise to async. It looks like you are replacing the promises with async whereas the problem was to ensure there are no floating promises. The number of changes are also much bigger because of that. Any reason to change it from promise to async ?

[naseemkullah]

I guess in many cases it would be enough to add catch instead of changing this from promise to catch. It looks like you are replacing the promises with async whereas the problem was to ensure there are no floating promises. The number of changes are also much bigger because of that. Any reason to change it from promise to async ?

Yeah good point, the only reason was my preference for async/await syntax but this does make the review process more cumbersome, I will revert those if that is preferred.

[naseemkullah]

I've made the rule warn in the test files where there are many floating promises, we can address these incrementally one (or more) test file(s) at a time in subsequent PRs.

[codecov]

Codecov Report

Merging #1780 (34a7bf9) into main (e27f9fb) will decrease coverage by 0.01%.
The diff coverage is 50.00%.

❗ Current head 34a7bf9 differs from pull request most recent head 075469a. Consider uploading reports for the commit 075469a to get more accurate results

@@            Coverage Diff             @@
##             main    #1780      +/-   ##
==========================================
- Coverage   92.75%   92.73%   -0.02%     
==========================================
  Files         138      138              
  Lines        4927     4928       +1     
  Branches     1017     1017              
==========================================
  Hits         4570     4570              
- Misses        357      358       +1     

Impacted Files	Coverage Δ
...ges/opentelemetry-metrics/src/export/Controller.ts	76.00% <0.00%> (-3.17%)	⬇️
...telemetry-tracing/src/export/BatchSpanProcessor.ts	91.20% <100.00%> (ø)

[dyladan]

I guess in many cases it would be enough to add catch instead of changing this from promise to catch. It looks like you are replacing the promises with async whereas the problem was to ensure there are no floating promises. The number of changes are also much bigger because of that. Any reason to change it from promise to async ?

Yeah good point, the only reason was my preference for async/await syntax but this does make the review process more cumbersome, I will revert those if that is preferred.

It is worth noting that the async/await syntax severely bloats the output size of the js. This is ok on the node side, but for the core modules and web modules it is something we need to consider.

[obecny]

instead of // eslint-disable-next-line @typescript-eslint/no-floating-promises I think you could either use some console logger with default log option or perhaps our global error handler ?

[naseemkullah]

instead of // eslint-disable-next-line @typescript-eslint/no-floating-promises I think you could either use some console logger with default log option or perhaps our global error handler ?

I should probably know this but what is our global error handler? diag.error() ?

[rauno56]

It is worth noting that the async/await syntax severely bloats the output size of the js. This is ok on the node side, but for the core modules and web modules it is something we need to consider.

I guess the bloat comes from the transpilation step, right?
Promises(and async/await, although probably a little bit less so) are widely supported everywhere except IE 11. Have we considered turning transpiling promises and/or async/await off altogether?

[dyladan]

instead of // eslint-disable-next-line @typescript-eslint/no-floating-promises I think you could either use some console logger with default log option or perhaps our global error handler ?

I should probably know this but what is our global error handler? diag.error() ?

There are 2 ways to handle errors. diag.error() will log them to whatever configured logger. There is also a global error handler which is called in cases where we would normally throw ourselves but prefer to have the user handle it. By default, it calls diag.error().

[dyladan]

Looks good to me pending the breaking lint step

[naseemkullah]

Ok, this is ready to go now. As mentioned we will tackle tests separately (the rule only emits a warning for tests until they are taken care of at which point we can enforce it).