Convert vis_type_tagcloud to Typescript

[flash1293]

The tag cloud visualization is still partially implemented in JS. These portions of the code should be converted to typescript to make it simpler to iterate and refactor

[elasticmachine]

Pinging @elastic/kibana-app (Team:KibanaApp)

[arjunvkurup]

Hi @flash1293 , I would like to start contributing to Kibana. Can I start working on solving this issue?

[flash1293]

Hi @arjunvijayanathakurup , it's great you want to participate! Contributors are always welcome 💚

This issue is not in progress already, feel free to work on it. I will assign it to you so nobody else will start working on the same thing.

[arjunvkurup]

Thank you @flash1293, really appreciate for assigning it to me.

[arjunvkurup]

Hi @flash1293 , I've been having some issues with the conversion. Can you help me out with them?

[flash1293]

@arjunvijayanathakurup sure, list them out here.

[arjunvkurup]

This is one of the errors, being shown by Es Lint on tag_cloud component

Second one with ORIENTATIONS on tag_cloud component

And on code:
tagCloudLayoutGenerator.fontSize(tag => mapSizeToFontSize(tag.value));
I'm getting an error saying value does not exist on d3TagCloud.Word

Property 'value' does not exist on type 'Word'.ts(2339)

[flash1293]

@arjunvijayanathakurup Hm, I'm not sure about the context here, it's probably related to other changes you did. Please put your current state in a draft PR, that makes it easier for me to reproduce.

[arjunvkurup]

@flash1293, sure I'll add them

[arjunvkurup]

Hey @flash1293, I have resolved some more errors but two remains. I got to know that d3.scale.linear() used in the code is outdated. It was supported on d3 type v3 and after that, it got changed to d3.scaleLinear() from d3 type v4 onwards. You can take a look at it.

[flash1293]

@arjunvijayanathakurup In such a case if it's just a single occurrence in the code it's fine to cast it to the matching type. I took a look at your draft PR and there are still quite some anys in there that can be typed more strictly.

Just a simple example: https://github.com/elastic/kibana/pull/64747/files#diff-4de8d7e461522a046f6fd82208d9fdd1R91 here visParams can be typed with VisParams from src/plugins/visualizations/public/vis.ts.

[arjunvkurup]

@flash1293, I will change them. I was unsure about the types for some of them, but I will look into it.

[arjunvkurup]

Hi @flash1293 , I have updated the code with the suggested changes.

[flash1293]

@arjunvijayanathakurup I went through parts of the PR, but there are still a lot of any types in there which are possible to type more strictly.

The reason I'm not feeling comfortable merging this in is that it gives the illusion of having type safety when in reality there is almost no protection. Leaving a few untyped parts in there because they are very difficult to type correctly is usually fine, but in the current state it's definitely too much to get merged, especially when the types are relatively straight forward to add.

A basic rule of thumb is to not have any types at all - and if there are some there should be a good reason for every single one.

[ashikmeerankutty]

@flash1293 Can I take this ?

[flash1293]

Hi @ashikmeerankutty , as you can see above @arjunvijayanathakurup started working on this a while back but wasn't active lately. I'm fine with you taking over, but you can also look into #65839 which is similar and nobody has started working on it yet.

[arjunvkurup]

Hi @flash1293 ,I'm sorry for not being active. I will be committing the latest today. I have already made several changes as per your directions.

[monfera]

Closing it as word cloud got ported and it uses TypeScript.