Skip to content

Commit

Permalink
feat: remove is-promise library (#2080)
Browse files Browse the repository at this point in the history
## 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
- [X] I have read
https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md
- [X] I have signed all commits
- [X] I have added unit tests for the new functionality
- [X] I have run lint and test steps successfully
  - for `jaeger`: `make lint test`
  - for `jaeger-ui`: `yarn lint` and `yarn test`

---------

Signed-off-by: Eshaan Aggarwal <96648934+EshaanAgg@users.noreply.github.com>
  • Loading branch information
EshaanAgg committed Jan 2, 2024
1 parent cdd251b commit 911ccf5
Show file tree
Hide file tree
Showing 7 changed files with 12 additions and 56 deletions.
1 change: 0 additions & 1 deletion packages/jaeger-ui/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,6 @@
"drange": "^2.0.0",
"global": "^4.3.2",
"history": "^4.6.3",
"is-promise": "^4.0.0",
"isomorphic-fetch": "^3.0.0",
"lodash": "^4.17.19",
"logfmt": "^1.4.0",
Expand Down
16 changes: 2 additions & 14 deletions packages/jaeger-ui/src/actions/jaeger-api.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,18 +21,6 @@ const metricType = {
errors: 'errors',
};

// export for tests
// TODO use native `allSetteled` once #818 is done
export function allSettled(promises) {
const wrappedPromises = promises.map(p =>
Promise.resolve(p).then(
val => ({ status: 'fulfilled', value: val }),
err => ({ status: 'rejected', reason: err })
)
);
return Promise.all(wrappedPromises);
}

export const fetchTrace = createAction(
'@JAEGER_API/FETCH_TRACE',
id => JaegerAPI.fetchTrace(id),
Expand Down Expand Up @@ -84,7 +72,7 @@ export const fetchDependencies = createAction('@JAEGER_API/FETCH_DEPENDENCIES',
export const fetchAllServiceMetrics = createAction(
'@JAEGER_API/FETCH_ALL_SERVICE_METRICS',
(serviceName, query) => {
return allSettled([
return Promise.allSettled([
JaegerAPI.fetchMetrics(metricType.latencies, [serviceName], { ...query, quantile: 0.5 }),
JaegerAPI.fetchMetrics(metricType.latencies, [serviceName], { ...query, quantile: 0.75 }),
JaegerAPI.fetchMetrics(metricType.latencies, [serviceName], query),
Expand All @@ -98,7 +86,7 @@ export const fetchAggregatedServiceMetrics = createAction(
'@JAEGER_API/FETCH_AGGREGATED_SERVICE_METRICS',
(serviceName, queryParams) => {
const query = { ...queryParams, groupByOperation: true };
return allSettled([
return Promise.allSettled([
JaegerAPI.fetchMetrics(metricType.latencies, [serviceName], query),
JaegerAPI.fetchMetrics(metricType.calls, [serviceName], query),
JaegerAPI.fetchMetrics(metricType.errors, [serviceName], query),
Expand Down
23 changes: 4 additions & 19 deletions packages/jaeger-ui/src/actions/jaeger-api.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,11 @@ jest.mock(
})
);

import sinon from 'sinon';
import isPromise from 'is-promise';
function isPromise(p) {
return p !== null && typeof p === 'object' && typeof p.then === 'function' && typeof p.catch === 'function';
}

import sinon from 'sinon';
import * as jaegerApiActions from './jaeger-api';
import JaegerAPI from '../api/jaeger';

Expand Down Expand Up @@ -174,20 +176,3 @@ describe('actions/jaeger-api', () => {
expect(() => mock.verify()).not.toThrow();
});
});

describe('allSettled', () => {
it('validate responses', async () => {
const res = await jaegerApiActions.allSettled([
Promise.resolve(1),
// eslint-disable-next-line prefer-promise-reject-errors
Promise.reject(2),
Promise.resolve(3),
]);

expect(res).toEqual([
{ status: 'fulfilled', value: 1 },
{ status: 'rejected', reason: 2 },
{ status: 'fulfilled', value: 3 },
]);
});
});
1 change: 1 addition & 0 deletions packages/jaeger-ui/src/utils/date.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -224,6 +224,7 @@ describe('formatRelativeDate', () => {
const output = input.toLocaleDateString('en-US', {
month: 'short',
day: 'numeric',
year: 'numeric',
});
expect(formatRelativeDate(input)).toBe(output);
});
Expand Down
9 changes: 0 additions & 9 deletions packages/jaeger-ui/src/utils/readJsonFile.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,18 +12,9 @@
// See the License for the specific language governing permissions and
// limitations under the License.

import isPromise from 'is-promise';

import readJsonFile from './readJsonFile';

describe('fileReader.readJsonFile', () => {
it('returns a promise', () => {
const promise = readJsonFile({ rando: true });
// prevent the unhandled rejection warning
promise.catch(() => {});
expect(isPromise(promise)).toBeTruthy();
});

it('rejects when given an invalid file', () => {
const p = readJsonFile({ rando: true });
return expect(p).rejects.toMatchObject(expect.any(Error));
Expand Down
13 changes: 5 additions & 8 deletions packages/jaeger-ui/src/utils/readJsonFile.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
// See the License for the specific language governing permissions and
// limitations under the License.

export default function readJsonFile(fileList: { file: File }) {
export default function readJsonFile(fileList: { file: File }): Promise<string> {
return new Promise((resolve, reject) => {
const reader = new FileReader();
reader.onload = () => {
Expand All @@ -22,24 +22,21 @@ export default function readJsonFile(fileList: { file: File }) {
}
try {
resolve(JSON.parse(reader.result));
/* eslint-disable @typescript-eslint/no-explicit-any */
} catch (error: any) {
reject(new Error(`Error parsing JSON: ${error.message}`));
} catch (error: unknown) {
reject(new Error(`Error parsing JSON: ${(error as Error).message}`));
}
};
reader.onerror = () => {
// eslint-disable-next-line no-console
const errMessage = reader.error ? `: ${String(reader.error)}` : '';
reject(new Error(`Error reading the JSON file${errMessage}`));
};
reader.onabort = () => {
// eslint-disable-next-line no-console
reject(new Error(`Reading the JSON file has been aborted`));
};
try {
reader.readAsText(fileList.file);
} catch (error: any) {
reject(new Error(`Error reading the JSON file: ${error.message}`));
} catch (error: unknown) {
reject(new Error(`Error reading the JSON file: ${(error as Error).message}`));
}
});
}
5 changes: 0 additions & 5 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -6110,11 +6110,6 @@ is-promise@^2.1.0:
version "2.1.0"
resolved "https://registry.yarnpkg.com/is-promise/-/is-promise-2.1.0.tgz#79a2a9ece7f096e80f36d2b2f3bc16c1ff4bf3fa"

is-promise@^4.0.0:
version "4.0.0"
resolved "https://registry.yarnpkg.com/is-promise/-/is-promise-4.0.0.tgz#42ff9f84206c1991d26debf520dd5c01042dd2f3"
integrity sha512-hvpoI6korhJMnej285dSg6nu1+e6uxs7zG3BYAm5byqDsgJNWwxzM6z6iZiAgQR4TJ30JmBTOwqZUw3WlyH3AQ==

is-regex@^1.0.4, is-regex@^1.0.5, is-regex@^1.1.4:
version "1.1.4"
resolved "https://registry.yarnpkg.com/is-regex/-/is-regex-1.1.4.tgz#eef5663cd59fa4c0ae339505323df6854bb15958"
Expand Down

0 comments on commit 911ccf5

Please sign in to comment.