feat: remove is-promise library

[EshaanAgg]

Which problem is this PR solving?

Fixes #2074

Description of the changes

• Removed the is-promise library
• Added the type assertions wherever necessary
• Removed the redundant tests
• Made use of the native Promise.allSettled function (as described in a comment)

There seems to be an unrelated test in date.test.js that fails after these changes. I have fixed the test with what I think the intended testing was supposed to do. Would appreciate a review on the same!

How was this change tested?

• Running the test suite locally

Checklist

• I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md
• I have signed all commits
• I have added unit tests for the new functionality
• I have run lint and test steps successfully
  • for jaeger: make lint test
  • for jaeger-ui: yarn lint and yarn test

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (cdd251b) 96.57% compared to head (8258c5c) 96.56%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2080      +/-   ##
==========================================
- Coverage   96.57%   96.56%   -0.02%     
==========================================
  Files         254      254              
  Lines        7626     7620       -6     
  Branches     1986     1986              
==========================================
- Hits         7365     7358       -7     
- Misses        261      262       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[yurishkuro]

Why are these tests removed? What is replacing them?

[EshaanAgg]

These are unnecessary tests. Each of these fetchXYZ functions is essentially createAction wrapper for different JSON files accessed via the get JSON function. Since the getJSON function returns a promise, the results of these calls are guaranteed to be PromiseLike. I removed these tests as they are essentially type-checking, which should be TypeScript's job.

[yurishkuro]

they are essentially type-checking, which should be TypeScript's job.

how is that type-checking achieved given that the source file in question is not in TS, but JS?

Also

Each of these fetchXYZ functions is essentially createAction wrapper

That's an implementation detail of those functions, but the tests were enforcing the public API. Implementations may change.

[EshaanAgg]

Ahh, my bad. I completely missed the .js extension. If we have to enforce the public API of these functions, then it would best to keep checking if we have a promise returned. I will revert the tests.

[EshaanAgg]

I removed this test as we have now deprecated the custom implementation of allSettled for the builtin Promise.allSettled.

[EshaanAgg]

I have reverted the tests testing the public API and removed the is-promise library in favor of a simple custom implementation that checks the supplied object must be PromiseLike, that is, have a then and catch function. I have removed the analogous tests for the readJsonFile function as the file is in TS, and we can use explicit return type assertions to ensure the same.

[yurishkuro]

Have you considered converting packages/jaeger-ui/src/actions/jaeger-api.js to Typescript?

[EshaanAgg]

I tried to do so but later decided to avoid doing it for two reasons:

• We would have to go back to our custom implementation of allSettled
  When changing the file to TS and using allSettled, we get the following TS error, asking us to increase the compiler to a more later one. Do you think this can lead to breaking changes?

Property 'allSettled' does not exist on type 'PromiseConstructor'. Do you need to change your target library? Try changing the 'lib' compiler option to 'es2020' or later. ts(2550)

• Loose types
  Even after flipping to TS, due to the nature of the redux-actions library, we get not very concrete types like the following:

const fetchAllServiceMetrics: ActionFunction<Action<any>, undefined, undefined, undefined, undefined>
const fetchDeepDependencyGraph: ActionFunctionAny<ActionMeta<any, {
    query: any;
}>>
const fetchServices: ActionFunction<Action<any>, undefined, undefined, undefined, undefined>

I don't think these types enforce the PromiseLike interface that you had earlier mentioned, thus making the exercise moot.

[yurishkuro]

Thanks!