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.

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

Modified getControl, getAttribute and get to return the basic type instead of undefined #294

Open
wants to merge 3 commits into
base: gh_master
Choose a base branch
from

Conversation

mathiasbl
Copy link
Contributor

My team start adopting strictest for TypeScript. https://www.npmjs.com/package/@tsconfig/strictest
It's doesn't like undefined alot as return type.

The getAttribute and getControl have been changed to return the basic type matching the method.

@mathiasbl mathiasbl changed the title Return the basic type instead of undefined Modified getControl, getAttribute and get to return the basic type instead of undefined Aug 25, 2023
@skovlund
Copy link
Contributor

Hi Mathias

I'm not familiar with strictest, so it is not obvious to me, what issue you are experiencing when using strictest with XDT. Is your issue with the XDT source code? Or with the generated typings?

We currently generate the typing getAttribute(attributeName: string): undefined as we believe it gives the best developer experience. Changing it to getAttribute(attributeName: string): any changes the compile-time error to a run-time error when using undefined fields. This means that removing a field (used in code) from a form no longer causes compile-time errors (this example does require re-generating the types, which we recommend doing that as part of your CI/CD pipeline).

Return type 'undefined' gives compile-time error

Return type 'any' gives run-time error

@mkholt
Copy link
Member

mkholt commented Nov 15, 2023

Agreed with @skovlund, we should not return a valid type for invalid method calls.

Would it be a better solution to let them return the never type?

@sideshowbob
Copy link

@mkholt According to the MS documentation, getControl returns an Object Collection, Object or null. Wouldn't it make sense to stick to the underlying SDK definition and at least return an XrmBaseControl, XrmBaseControl[] or null?

@jbrueschweileravantic

@mkholt
Copy link
Member

mkholt commented May 2, 2024

@sideshowbob, it seems both getControl and getAttribute return null if the control / attribute is not on the entity.
So we could return null as a fallback value as well.

My point in using never was that we want to avoid you ever calling using an identifier we know doesn't exist.
Specifically, we should return something that will result in compile-time errors.

@sideshowbob
Copy link

@mkholt I see your point for resulting compile-time errors. But there are circumstances where you may explicitly want to get a control based on, say a variable that is populate at run-time. From my point of view, this prevents some generic development approaches and does not correspond with the SDK, the Types are made for.

@mkholt
Copy link
Member

mkholt commented May 2, 2024

@sideshowbob, part of the intention behind the framework is also to provide compile-time safety.

I believe you would be able to do a cast if you wish to do something unsafe, which you would then show your intention of doing by doing the cast, e.g. getControl('some_control') as unknown as Xrm.CustomControl

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants