You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
/* This is my first ever contribution to GH, so forgive my non-standard way of Starting a Discussion. Thanks in advance! */
Issue in @builder/sdk
TLDR: builder.get() and builder.getAll() should accept a Generic Type to determine the Object shape of Custom Component, content models, etc. to avoid type assertion hackaround downstream.
Background - In our organization, we use a lot of custom components, content models in NextJS Framework, and .get() and .getAll() functions are used all the time to get the data for these components. These methods return BehaviorSubject<any, any> and Promise<BuilderContent[]> respectively.
The first 'real' rule of TS in production environments is - "No matter what happens, do not use 'any'. We did not go through all the hardships of setting up Typescript and related protections, and for a developer to just ignore all of it by using 'any'". (I know we can force a 'compilation' error, but that is not the point here).
Keeping that in mind, what bugs me a lot is that Builder for some reason does not provide any mechanism to determine the shape of the return value of those two functions mentioned earlier, and defaults it to 'any'. PFB the typings -
Here, BuilderContent['data'] has a placeholder [key: string]: any;
Now, my question is - why? Why does this defaults to any, and why have they not just given us the ability to provide custom types via a Generic Type to BuilderContent, because at the end of the day, both .get() and .getAll() do return either a BuilderContent or a BuilderContent[].
If you dig deep in builder\packages\core\src\builder.class.ts, which is not that hard to do, you will see that they explicitly have used any in their logic. It does makes sense in the SDK context, as they have no idea what the return object is, and they should not care. But, for us, the end users of this SDK have to suffer through setting up custom types to resolve this any to something meaningful which can be used downstream.. well.. meaningfully.
BDW, this is what I had to create for a meaningful intellisense, and use it to define a variable holding the return value of .get() -
type BuilderResponse<Model> = Omit<BuilderContent, "data"> & { data?: Model & Omit<BuilderContent["data"], "[key:string]">; }
And it still does not work as it is supposed to.
This can easily be avoided if Builder simply provides ability to add Generic return types to these functions, and similarly to other functions which return 'any', because as mentioned earlier, they return BuilderContent in one way or another.
Maybe I am totally out of the line for mentioning this issue, and maybe I am the only one who does not like hackarounds of inferred types and custom type assertions, but as a developer using Builder SDK, I would like to see this fixed on a fundamental level, and I am ready to contribute to this, if given the chance.
<<END OF RANT>>
The text was updated successfully, but these errors were encountered:
/* This is my first ever contribution to GH, so forgive my non-standard way of Starting a Discussion. Thanks in advance! */
Issue in
@builder/sdk
TLDR:
builder.get()
andbuilder.getAll()
should accept a Generic Type to determine the Object shape of Custom Component, content models, etc. to avoid type assertion hackaround downstream.Background - In our organization, we use a lot of custom components, content models in NextJS Framework, and
.get()
and.getAll()
functions are used all the time to get the data for these components. These methods returnBehaviorSubject<any, any>
andPromise<BuilderContent[]>
respectively.The first 'real' rule of TS in production environments is - "No matter what happens, do not use '
any
'. We did not go through all the hardships of setting up Typescript and related protections, and for a developer to just ignore all of it by using 'any
'". (I know we can force a 'compilation' error, but that is not the point here).Keeping that in mind, what bugs me a lot is that Builder for some reason does not provide any mechanism to determine the shape of the return value of those two functions mentioned earlier, and defaults it to '
any
'. PFB the typings -@builder.io\sdk\dist\src\builder.class.d.ts -
get(modelName: string, options?: GetContentOptions & { req?: IncomingMessage; res?: ServerResponse; apiKey?: string; authToken?: string; }): BehaviorSubject<any, any>;
getAll(modelName: string, options?: GetContentOptions & { req?: IncomingMessage; res?: ServerResponse; apiKey?: string; authToken?: string; }): Promise<BuilderContent[]>
Here,
BuilderContent['data']
has a placeholder[key: string]: any;
Now, my question is - why? Why does this defaults to any, and why have they not just given us the ability to provide custom types via a Generic Type to BuilderContent, because at the end of the day, both
.get()
and.getAll()
do return either aBuilderContent
or aBuilderContent[]
.If you dig deep in builder\packages\core\src\builder.class.ts, which is not that hard to do, you will see that they explicitly have used
any
in their logic. It does makes sense in the SDK context, as they have no idea what the return object is, and they should not care. But, for us, the end users of this SDK have to suffer through setting up custom types to resolve thisany
to something meaningful which can be used downstream.. well.. meaningfully.BDW, this is what I had to create for a meaningful intellisense, and use it to define a variable holding the return value of .get() -
type BuilderResponse<Model> = Omit<BuilderContent, "data"> & { data?: Model & Omit<BuilderContent["data"], "[key:string]">; }
And it still does not work as it is supposed to.
This can easily be avoided if Builder simply provides ability to add Generic return types to these functions, and similarly to other functions which return '
any
', because as mentioned earlier, they return BuilderContent in one way or another.Maybe I am totally out of the line for mentioning this issue, and maybe I am the only one who does not like hackarounds of inferred types and custom type assertions, but as a developer using Builder SDK, I would like to see this fixed on a fundamental level, and I am ready to contribute to this, if given the chance.
<<END OF RANT>>
The text was updated successfully, but these errors were encountered: