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

Suggestion #1

Closed
themike10452 opened this issue May 10, 2017 · 7 comments
Closed

Suggestion #1

themike10452 opened this issue May 10, 2017 · 7 comments

Comments

@themike10452
Copy link

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
Copy link
Collaborator

mktange commented May 11, 2017

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 mktange self-assigned this May 11, 2017
@mktange
Copy link
Collaborator

mktange commented May 16, 2017

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.

@mktange mktange closed this as completed May 16, 2017
@mktange mktange removed their assignment May 16, 2017
@themike10452
Copy link
Author

themike10452 commented May 18, 2017

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
Copy link
Collaborator

mktange commented May 18, 2017

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
Copy link
Author

themike10452 commented May 24, 2017

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
Copy link
Collaborator

mktange commented May 24, 2017

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
Copy link
Collaborator

mktange commented May 25, 2017

@themike10452 latest patch release should fix this issue

magesoe pushed a commit that referenced this issue Mar 11, 2019
magesoe pushed a commit that referenced this issue Jan 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants