(fix): Improved immutable state for variable and parallel execution for jsonFilesText. #704
Conversation
…or jsonFilesText.
|
@vishal2612200 is attempting to deploy a commit to the thirdweb Team on Vercel. A member of the Team first needs to authorize it. |
There was a problem hiding this comment.
hey @vishal2612200 thanks for the PR!
I've left some comments for further improvements that I think we could make on this based on your changes.
In general I believe the promise all approach to parallelizing the json file reading is a good perf win, the pipeFunctionExecutor function (while logical for this use-case) makes the code harder to reason about that it was before and I would like to avoid that.
| export const removeEmptyKeysFromObject = (obj: any) => | ||
| Object.keys(obj).reduce((result, key) => { | ||
| if (!obj(key) || obj(key).length === 0) return result; | ||
| return { ...result, [key]: obj[key] }; | ||
| }, {} as any); |
There was a problem hiding this comment.
we started out with any here so not necessary but with this change I believe we can improve the types of this quite a bit by using a generic input type that we remove keys from for the return type based on the same rules as during runtime, something along the lines of:
input: TInputObject extends object
output: Exclude<TInputObject, [keys that are nullish | that are array and have length of 0]>
| return pipeFunctionExecutor( | ||
| constructAttributesFromObj, | ||
| removeEmptyValues, | ||
| )(obj); |
There was a problem hiding this comment.
I conceptually like it but I have a problem with how much harder it makes this to read / understand what's going on at a glance, I'd prefer to keep the nested functions for this a la convertToOsStandard(removeEmptyKeysFromObject(properties))
| export const pipeFunctionExecutor = | ||
| (...fns: any) => | ||
| (...value: any) => | ||
| fns.reduce((result: any, fn: any) => fn(result), ...value); |
There was a problem hiding this comment.
if we end up going wit this this definitely needs better types than any so it's easier to understand the data flow of this and reason about it when using it
There was a problem hiding this comment.
agree with you, we will need better type than any, I was not sure about type over here, so decided to use any over here.
|
Thanks @jnsdls for comments, I will add your suggested changes in next commit. |
Fix List