Port to TypeScript (DONT MERGE)

[KaanMol]

Trying to port Iro.JS to Iro.TS (funny right?).

The first stage is make Iro.JS compileable.
After that the codebase will get improvements.
If somebody has any suggestions, just throw them at me.

• Is compileable
• Works (sort of)
• Add correct types everywhere
• Make buildable with Microbundle
• (optional) Rebuild the whole codebase to make more sense with TS

[KaanMol]

Currently it looks like everything works? I dont know for sure until I test everything first.
And from this point it is just cleaning everything up, giving everything the correct type and that should be it?

And removing all the bullshit I added to make it work quickly ofcourse

[jaames]

Hey @KaanMol, thanks so much for your work on this! I'm currently away over the weekend so I'll get back to you with a thorough review on Tuesday, hope thats okay :)

[KaanMol]

@jaames Ofcourse James, it still isn't finished so take your time! I still have a lot to do anyway

[jaames]

Just got the chance to review this, it's a pretty good start! A couple of things I wanted to note:

• I'd like to keep using the current rollup build process for now, and integrate rollup-plugin-typescript2 to handle typescript. The config files from tsdx and microbundle will probably be helpful for reference

• I see you're using the react typedefs, although iro.js uses preact. While they do share a pretty similar API, there some differences so it would probably best to use preact-specific typedefs. IIRC preact provides them out-of-the-box but I might be wrong

• The wheel and slider components inherit all the props passed to the colorpicker, so they should probably inherit the prop types from it too

• The jest test suite probably needs tweaking to support typescript, I think tsdx & microbundle might be helpful as reference here too

I'm happy to tackle any of these though, so don't worry too much about it

Thanks again for the contribution! :)

[KaanMol]

Hey James,

I used this buildprocess to quickly prototype it, I am going to remove that after I am done.

You are correct, I hadn't used preact and the TSC was complaining about typedefs which it couldn't find so I tried several things. The React Typedefs aren't used and will even cause problems if you install them. I have removed them locally. When I made a clean install of my node_modules folder, the preact typedefs were automatically found.

Yeah you are right about inheriting proptypes, I still need to clean up a LOT and have to set the correct types everywhere. My goal was just to get it working quickly in TypeScript.

I saw that you sometimes change the type of a variable. It is better to keep 1 type for a variable instead of changing it sometimes from lets say a number to a boolean.
We are actually really far from home, since this is still "messy typescript".

Have you worked with strongly typed languages before? If you did, then we can probably rewrite all the parts easily together :)

[jaames]

Fair enough!

I totally agree that some code cleanup is needed. Actually, in general a lot of things haven't really changed since they were first implemented, and in the past I was using lots of tricks to try to squeeze every last byte out of the minified bundle size which isn't as much of a concern now. Out of curiosity, are there any parts that you feel are particularly messy at the moment?

And yeah, I am familiar with strong typing. I don't think it will be too difficult to keep variable types consistent, we should just be careful with color picker props because some (e.g. layout and sliderHeight) are overridden with internal logic if they are not defined

[KaanMol]

Sorry for my late reaction, this week is just really busy. After tomorrow I will have a lot more time.

We can still try to squeeze every last byte to make the package really small. I think that we can even make it smaller than the package currently is, but not much tho, maybe a really small bit.

There isn't really "one" part that feels messy. The structure feels messy or something, every file for itself looks not horrible, but everything combined looks weird and messy. When I started porting, I had a log of issues, there were properties and functions coming out of nowhere.

Do you mean overridden with internal logic by preact or by Iro.JS? If it is PReact, don't worry about that. If it is by Iro.JS, we can probably find a way to fix it.

[jaames]

No worries! Take your time, I'm a little busy too

Like the file structure itself is messy? Or the way that things are shared across files? I don't really have a computer science background or anything like that, so I suppose there may be some strange choices. I'm happy to hear any suggestions if you think there's a better way to organise it though

Oh -- they're overridden by logic in iro.js. For example if the user doesn't provide a sliderHeight then it is calculated from the handle radius, padding, etc.

[KaanMol]

To be fair, the choices aren't that bad, it is always just somebody's opinion. The file structure is not that bad, the messy parts are just the inconsistencies across the code.
Things like for example the files in the "util" folder, why is one file called "colorUtils" and the other one "dom" instead of "domUtils".
But don't worry about that, I am horrible too at staying consistent across my projects, I believe everybody is.

Ahh, but a sliderHeight will always be a variable of the type Number so if Iro overrides it, it is overriding it by still giving it a Number, right? So it won't be a problem. And if it is a problem, we will find a way, don't worry.

You said that a lot of things didn't change from the beginning and that you would like a cleanup. Which parts aren't that changed and really need a cleanup in your opinion? Lets discuss how we are going to clean it up, before actually cleaning it up.

[jaames]

Hey, sorry again for the slow response

I can see how that feels confusing, I'd support changing those to something more consistent :)

Well, so far I've been using null as the default value for sliderHeight, layout, etc, and they're only overridden if the prop is set to that default value. We just need to take a little extra care to make sure that behaviour doesn't break if the prop always has a consistent type.

Cleanup-wise there's a couple of things:

• utils/dom is very old and isn't really needed anymore. onDocumentReady can be inlined into utils/createWidget, and listen / unlisten could go directly into ui/Component, which is the only place they're used. We could also consider doing the same with the other utils.

• There's 3 similar event-like systems in colorPicker -- emit for public events, emitHook for plugin hooks and deferredEmit for deferred public events. Could these perhaps be cleaned up to share code somehow?

• I'm not sure that extending Preact's component class in ui/Component was necessary, the functionality that it provides to ui components could be implemented as higher order component instead. But I'd need to think about that more.

[KaanMol]

Hey,

null is not a different type. In languages like C++ you can set variables for example to NULL. What NULL means is that the variable gets initialized, with all zero's in the memory. In Javascript it probably doesn't mean this. But I mean with this whole story that if the other type is null that there isn't a problem, as long as string's don't change to numbers or vice versa without proper casting.

I don't know for sure how we could combine the 3 event-like systems right now. It should be possible I think. Going to look in to it to be sure.

Hmm, yeah about the extending part, I see why you did it. We could change that, depends if you want it. Let me know.

[jaames]

I did some research and it seems you're right, setting a typed variable to null is acceptable Typescript so long as the config strictNullChecks flag is false.

How do you want to organise collaborating on this project by the way? Should I just push to your fork or should I set up a new branch here? I need to port over the new id param from iro.js 4.5.0: 86034e9

Also sorry for the slow reply, I've been throwing a lot of time and energy into this project recently and I'm starting to feel a little burnt out tbh.

[KaanMol]

Oh hey, really sorry, I thought that you gave up on me hahah.
Maybe it is a better idea to merge this in a new branch, and I will just make PR's to that branch.

No worries, I totally get it, I have been focusing on exams etc lately.
Don't worry if it is all a bit too much :)

[jaames]

Ahah, sorry about that!

I've merged this into iro.js typescript branch, I had to resolve a couple of merge conflicts but feature-wise the typescript port should be identical to the latest version of iro.js now

Since this is technically merged now, would you like to continue the conversation here or should we move to an issue thread or something?

[KaanMol]

Hahah, no worries (:

Yeah feature-wise it isn't 100% identical.
I know that I disabled something about alpha colors, but that is not a real issue.

Maybe an issue thread is better, I have an idea which should make iro.js smaller and easier to port to frameworks or even for vanillaJS