Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Update non-major-dev-dependencies #367

Merged
merged 3 commits into from
Sep 3, 2024
Merged

Update non-major-dev-dependencies #367

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b7d8e4d
Update non-major-dev-dependencies
renovate[bot] Sep 3, 2024
64df5ec
Utilize immediately invoked function for promise handling
bdrhn9 Sep 3, 2024
d6dc22d
Run eslint
bdrhn9 Sep 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 6 additions & 8 deletions src/update-feeds-loops/update-feeds-loops.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,7 @@ export const runUpdateFeeds = async (providerName: string, chain: Chain, chainId
provider,
chainId,
firstBatchBlockNumber
).catch((error) => error);
).catch((error: any) => error);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In new version 0.6.1 of @total-typescript/ts-reset, catch resolves output as unknown rather than any.


// Calculate the stagger time.
const batchesCount = Math.ceil(activeDataFeedCount! / dataFeedBatchSize);
Expand Down Expand Up @@ -226,13 +226,11 @@ export const runUpdateFeeds = async (providerName: string, chain: Chain, chainId

// Wait for all the batches to be processed and print stats from this run.
const processedBatches = await Promise.all([
new Promise<Awaited<ReturnType<typeof processBatch>>>((resolve, reject) => {
return processFirstBatchPromise.then((result) => {
// eslint-disable-next-line promise/always-return
if (isError(result)) reject(result);
else resolve(result);
});
}),
(async (): Promise<Awaited<ReturnType<typeof processBatch>>> => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made these changes in response to an issue caused by upgrading @total-typescript/ts-reset from version 0.5.1 to 0.6.1, which resulted in TypeScript compilation errors. The update introduced stricter type-checking that required a more explicit approach to handling types and promises.

This immediately invoked function approach also improves code readability.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TBH: I'm not a huge fan of IIFE in regards to readability.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. So, would you prefer to use previous approach or do you have better suggestion on this? I can change this.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nah, I think it's not worth to change this. I think I'd prefer the previous approach (but not sure what was the TS issue there).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mostly wanted express my opinion on that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but not sure what was the TS issue there

The error was for this part:

error: src/update-feeds-loops/update-feeds-loops.ts:230:45 - error TS2349: This expression is not callable.
  Each member of the union type '{ <TResult1 = { signedApiUrlsFromConfig: string[]; signedApiUrlsFromContract: string[]; beaconIds: 0x${string}[]; successCount: number; errorCount: number; }, TResult2 = never>(onfulfilled?: ((value: { signedApiUrlsFromConfig: string[]; signedApiUrlsFromContract: string[]; beaconIds: 0x${string}[]; successCount:...' has signatures, but none of those signatures are compatible with each other.

230             return processFirstBatchPromise.then((result) => {
                                                ~~~~

src/update-feeds-loops/update-feeds-loops.ts:230:51 - error TS7006: Parameter 'result' implicitly has an 'any' type.

230             return processFirstBatchPromise.then((result) => {
                                                      ~~~~~~

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mhh, strange. Just from the looks of it I don't know what is wrong. But I think you're solution is good OK, so wouldn't bother investigating further.

const batchOrError = await processFirstBatchPromise;
if (isError(batchOrError)) throw batchOrError;
return batchOrError;
})(),
...processOtherBatchesPromises,
]);

Expand Down