delegateas / XrmDefinitelyTyped

Tool to generate TypeScript declaration files for Dynamics 365/CDS client-side coding.
http://delegateas.github.io/Delegate.XrmDefinitelyTyped/
MIT License
133 stars 53 forks source link

Suggestion #1

Closed themike10452 closed 7 years ago

themike10452 commented 7 years ago

Hello there, first I would like to thank you for this awesome tool.

I have a small suggestion; is it possible to add an option to omit get(name: string): Xrm.EmptyAttribute; and get(name: string): Xrm.EmptyControl; that way we can only use valid attribute/control names with Xrm.Page.getAttribute and Xrm.Page.getControl.

What do you think?

mktange commented 7 years ago

It was originally implemented like this because it was the only way to have specific string overloads in function definitions. It required you to have a default/catch-all case of type string after all the specific strings.

But with the addition of string literals in the later TypeScript versions, it is likely possible that it will work without this catch-all function definition anymore. If this is the case, I can get rid of these additional functions definitions along with EmptyAttribute and EmptyControl all together.

I will be looking into the possibility of removing them.

Good suggestion, thanks!

mktange commented 7 years ago

When there is no default case, TypeScript resorts to showing the following error: Imgur Here "websiteurl" just so happens to be the last option for that specific getAttribute-function call.

And if you try to go on, the compiler won't complain and just assumes that you meant to type "websiteurl". It will then continue giving the user autocompletion as if that is what was written: Imgur

Given these two observations, I won't be making this change at the moment, since it will lead to a worse user experience in my opinion.

There is another option that I might be exploring soon. It includes making the functions generic, and utilizing the keyof function in TypeScript. This is, however, a bigger change to the code, and I'm still not quite sure that the user experience will be better with this approach either.

themike10452 commented 7 years ago

Thank you for your time mate. Is there any actual use of EmptyAttribute? It is impossible to set/get value of EmptyAttribute (without explicitly casting to StringAttribute for example) which made me wonder why it existed in the first place. Casting getAttribute to a specific type does the job, but it looks ugly var value = (<Xrm.StringAttribute>Page.getAttribute("doesnotexist")).getValue()

This looks much better in my opinion: var value = Page.getAttribute<StringAttribute>("doesnotexist").getValue() or var value = Page.getAttribute("doesnotexist").getValue<String>()

But again, since the script is generating typings for all existing fields there wont be a case when we're actually using this overload anyway.

As for the screenshots you posted above, we can only blame the poor intellisense of VS, I am using resharper and this is what I get 😃

image

Just thinking loudly here. Thank you for your efforts!

mktange commented 7 years ago

The whole point behind XrmDefinitelyTyped for forms is that the type of an attribute/control should be inferred from the string given to the getter function. It should not be possible to request it with a generic version of that function, since this will defeat the whole purpose of eliminating potential bugs, that arise from using incorrect strings.

The EmptyAttribute/EmptyControl are present to indicate to the user that the used string is not valid on that form. They are called empty, because they are empty interfaces without any functionality, and thus the developer is not able to use it to do anything (as they should not be, since that element is not present on the targeted form).

In an upcoming minor version, I may be replacing all EmptyXyz interfaces with the never key word, since these cases should never happen.

Regarding the more descriptive Resharper error message: It does look nicer, but this tool should not be reliant on developers using certain third-party extensions. The goal for this tool is to provide a good developer experience using TypeScript for CRM regardless of IDE and external tooling.

themike10452 commented 7 years ago

I absolutely agree with the last part, I was only pointing out the beauty of Resharper.

One last thing, why is Xrm.FormType enumeration private? Is there another enumeration that we could use?

Edit: I am using declare var Xrm: Xrm<Form.entityname.Main.Information> which is hiding FormType enumeration from Xrm namespace.

mktange commented 7 years ago

Oh yeah, this issue was mentioned to me by a co-worker a while back, but I have not gotten around to fixing it yet.

The issue only arises when you use the declare var Xrm method when using form interfaces, and became a problem when I changed the name of the general namespace from IPage to Xrm with the 2.0 version. This re-declaration of the Xrm namespace/variable overwrites the previous values found in the Xrm-namespace, like Xrm.FormType.

I will be looking into fixing this soon.

mktange commented 7 years ago

@themike10452 latest patch release should fix this issue