fix: nested ctype loading #931
Conversation
|
@aybarsayan I moved nested CType loading to |
| } | ||
|
|
||
| // Process all values in the object. Also works for arrays | ||
| await Promise.all(Object.values(value).map(extractRefsFrom)) |
There was a problem hiding this comment.
I had asked chat about this.
Not sure if we care to much about proformance. However, it gave me an interesting insight into this Promise.all and map.
await Promise.all(Object.values(value).map(extractRefsFrom))- The issue is that extractRefsFrom is async, but map() does not handle async functions well—it creates a list of
Promise<void>, which Promise.all then waits for. - While this works in many cases, it may cause performance issues if many deep CType references are being processed simultaneously.
- Solution: You may consider sequential processing for deeply nested structures using for...of:
for (const subValue of Object.values(value)) {
await extractRefsFrom(subValue)
}This prevents too many concurrent fetches.
There was a problem hiding this comment.
Bizarre that chad thinks performance would be better if we process them sequentially. Async code exists exactly for this use case, where it doesn't make much sense to block execution while waiting for the full node to return historic blocks. In case there are issues with making too many requests at the same time, I'd recommend rate limiting on the ctype loader side, but not here.
There was a problem hiding this comment.
Yes, it is strange. There wasn't any issues on my side, but I like to run it through chat afterwards. Seeing what interesting things it comes up with. I sent the test results. It basically a pointless comment without the context of what we are doing in the project.
| @@ -32,9 +37,69 @@ export function newCachingCTypeLoader( | |||
| }) | |||
|
|
|||
| async function getCType(id: ICType['$id']): Promise<ICType> { | |||
There was a problem hiding this comment.
Randomly adding more soundness checks?
There was a problem hiding this comment.
I moved them from the code for loading nested ctype definitions, as they seemed misplaced there.
| } | ||
|
|
||
| // Process all values in the object. Also works for arrays | ||
| await Promise.all(Object.values(value).map(extractRefsFrom)) |
There was a problem hiding this comment.
Yes, it is strange. There wasn't any issues on my side, but I like to run it through chat afterwards. Seeing what interesting things it comes up with. I sent the test results. It basically a pointless comment without the context of what we are doing in the project.
| @@ -32,9 +37,69 @@ export function newCachingCTypeLoader( | |||
| }) | |||
|
|
|||
| async function getCType(id: ICType['$id']): Promise<ICType> { | |||
Fixes https://github.com/KILTprotocol/ticket/issues/3632
fork of #919
Integration tests are not currently working (#944)
Checklist: