Refactor: Convert into TypeScript

[rschristian]

Currently just a first draft of a TS conversion, mentioned first here. Currently still a few issues, some of which just need time, some which will require a few changes.

How would you feel about merging much of the files in utils/ into a single file, say conversions.ts? As of now it's quite tedious to go through the dozen or so files. Named exports would vastly reduce the amount of files making for a much better developer experience IMO.

The build config has a few problems revolving around type generation, not entirely sure how to fix that yet, if possible. Microbundle is really fighting on type definition generation. Adding the types field to the package.json ensures the main export (dist) has generated files and all users can use them. But for some reason that stops all the other exports from generating any types whatsoever. Not sure what's going on there, will have to look into it more.

Have you thought about not splitting up your library into a bunch of separate modules? With modern tree-shaking, I think it could be beneficial to just export one bundle of named components and have users import the ones they want to use. That would result in the following Microbundle output:

Build "reactColorful" to dist:
       2.1 kB: index.js.gz
      1.86 kB: index.js.br
      2.04 kB: index.esmodule.js.gz
      1.82 kB: index.esmodule.js.br
      2.11 kB: index.module.js.gz
      1.87 kB: index.module.js.br
      2.17 kB: index.umd.js.gz
      1.93 kB: index.umd.js.br

With the following API:

import { HexColorPicker } from 'react-colorful';

Bit bigger, but whatever you don't use will be shaken out with modern build tools.

It would vastly reduce your build process complexity and package size in exchange for some bundle size for the users not using build tools. Just a thought.

[omgovich]

What a massive work you did! Thank you!
Sorry, don't have time look closer today. But will do that tomorrow.

Regarding:

Have you thought about not splitting up your library into a bunch of separate modules?

That's the thing I want to avoid because tree-shaking doesn't always work properly.

Sometimes it doesn't work at all. For example, Parcel bundler still doesn't have a stable version of the tree-shaking feature.

[rschristian]

Sorry, didn't clarify.

Any use of any was slated to get fixed up, same with some of my (admittedly poor) first-pass interface names. Was just trying to get something to compile first.

[rschristian]

As I mentioned, there are a few issues with the current build system, resulting in Microbundle never generating types for the non-default modules. This very well might be a bug, and I'll raise an issue. Regardless though, I've been thinking about rewriting it in a form that would most closely resemble Preact's setup for a few reasons.

Using the current system is quite limiting as Microbundle has some options that are only configured via a package.json file, and as one is dynamically generated here, they can not be taken advantage of. For example, build outputs. Type outputs might be one of these, though again, this might very well be a bug.

Moving to what I suggest not only opens up a couple more Microbundle options but would also allow for targeted builds (rebuilding just hex) and reduce build complexity as build-packages.js could entirely be deprecated. Not that it is a monster of a build script, but passing off the file system manipulation and the like to Microbundle would reduce surface area for maintenance.

We would simply track hsv, rgbString, etc. and place a static package.json file within them. All built code would be put into {hsv,rgbString}/dist rather than just {hsv,rgbString}. They can just as easily still point into the src directory for their source files so Microbundle won't need any extra help.

Might whip up a quick demo using the current state of master (so non-ts). This PR certainly doesn't need more things being done in it.

[rschristian]

@omgovich I updated the build process, everything seems to be good.

I did switch it over to use ESModules, as I figure that better matches the repository with the ES6 import syntax and the like. Let me know if you'd rather keep it as CommonJS though, no problem. Thought I'd give it a shot.

I did have to enable an experimental flag to import from a .json file for getting the peerDependencies field but I figure that's unlikely to cause issues.

Edit: Didn't notice this failed for older Node. I'll fix tomorrow.

[rschristian]

Also, something you may notice is that TSC/Microbundle will output types for all files, even those that are not actually public interfaces. Unfortunately there's no way to stop this besides manual pruning. TS can't yet distinguish between external and internal typings.

So in our bundle for say HexInput will be the typings for hex, as well as Interaction. Neither are public, neither are accessible, yet they're shipped out in the bundle. This shouldn't actually matter as no types will ever be shipped to end users, but I digress.

Maybe this only bothers me though. Don't know. Thought it worth mentioning that, yes, technically it is working as intended.

[rschristian]

@omgovich

Should be good to go now. Of course still review away if I've missed something.

[omgovich]

@molefrog Could you join us here? Fresh eyes would be good)

[omgovich]

Works and looks good!

@ryanchristian4427 Could I ask you to fix conflicts?

In my opinion, this is how our plan should look like:

1. Fix conflicts
2. Deploy to NPM with beta-label — v2.3.0-beta.1
3. Install the beta-package to some TS-projects and test how it works in a real environment.
4. Merge with brand to master and release v2.3.0

@ryanchristian4427 Sounds good?

[rschristian]

Sorry, conflicts? Not seeing any on my side.

But rather than publish a whole beta version, we could just use npm/yarn link and test that way? That's what I've been doing as I've went along, testing the components/intellisense typings in Preact TS project.

I should still do another full run through, but I don't think there's a need for a beta?

[omgovich]

Guess you're right. We could just use link instead.

[omgovich]

Oh, sorry. Everything is okay — there are no conflicts.

[rschristian]

I'm wondering if we should add @types/react to the peer dependencies list. While many projects do have the skipLibCheck in their tsconfig files, some do not. You can just as easily ignore peer dependency warnings but this would be a nice reminder for say TS Preact users.

Edit: To expound, TS does not have any ability to alias. While most Preact applications will alias React to Preact, ensuring this library works just as well in Preact, when it comes to TS, there's no way around it. If they don't want to skip lib checks then a Preact application will need to have @types/react installed otherwise there will be an error when building.

[rschristian]

Also, I know next to nothing about HSL/HSV so while things look a bit weird to me they might not be? Might just be the wheel -> square conversion, but values don't seem to increase/decrease like I'd expect.

[rschristian]

Additionally, how do you feel about maybe doing named exports with the types? For example:

export default HslColorPicker;
export { HSL };

import HslColorPicker, { HSL } from 'react-colorful/hsl';

They could otherwise import the type like import { HSL } from 'react-colorful/dist/types'; or import { HSL } from 'react-colorful/src/types'; but I'd say those aren't as good of experiences.

[omgovich]

@ryanchristian4427 Merged. Thank you again!

P.S. I love the idea of the named color types export!

[rschristian]

@omgovich

I'm going to actually fix our external/internal definitions for the three (RGB, HSL, HSV) first. I realized exporting those types would make it a bit more obvious that any string key could actually be added and would work fine (i.e., { r: 0, g: 0, b: 0, x: 0 }).

As of now the user has to more or less dig to find out what HSV is strictly defined as, which almost is a benefit as most wouldn't think of adding extra keys to an HSV object. Not the best situation but I'll try to straighten it out. Should just be able to cast to a different object when doing our color object comparisons, and keep otherwise correct types throughout.

[rschristian]

Regarding:

Have you thought about not splitting up your library into a bunch of separate modules?

That's the thing I want to avoid because tree-shaking doesn't always work properly.

Sometimes it doesn't work at all. For example, Parcel bundler still doesn't have a stable version of the tree-shaking feature.

I'm still thinking the output should be combined and user can use named exports to get the parts of the library that they want. While multiple separate inputs -> outputs is on Microbundle's roadmap I don't see any related PRs open (or closed) relating to it. Just old issues.

To continue using the current solution would just be a nightmare I think, especially as time goes on. Every extra output that gets added means more duplicated .d.ts files, more duplicated .css files, etc. Maybe there are ways to prune these but that just moves the complexity into your build script. Not to mention build time, which is quite slow as well. Seeing as how we can't selectively target modules for compilation (at least not without also generating tsconfig files as build time to narrow compilation scope) each new file is also going to exponentially increase build times.

The current worst case scenario is if user wants to use only HexInput with 0 tree-shaking of any kind, as odd as that'd be. That's still just 1.6kb of potential waste. And while I know, just?!, but yes, just. Because that is absolutely nothing compared to the average React app (CRA with TS starts at 120kb). It's minimal even to the average Preact app. Of course, in a more realistic scenario, no one would be trying to use the HexInput and the HexInput alone. It's pretty much just a normal input element. Of the remaining components, the smallest is the HsvColorPicker. If a user wanted to use only it, with 0 tree-shaking, they'd see an 850b bundle size increase. Which is even smaller of an issue.

As was said more than a year ago now, the popular bundlers will all handle tree-shaking. If Parcel/another bundler can't work this out I think that's an issue for them, and not something we should really worry about. Majority of users use Webpack, and it can tree-shake quite well.

The move to named exports will see:

• Faster build times
  • Could be important if more people want to help with this library
• Smaller overall bundles (both in file size & count)
• Less surface area for maintenance, as the build script/s won't be needed
  • Again, useful if more people want to help
• No need for irrelevant files (types for say the hex color picker in the hsv's output, or stylesheets in anything but the main output)
• Slightly less verbose API, as now I can import both HexColorPicker and HexInput in the same statement
• No increase to bundle size with all modern (and widely used) build tools

Working with this library more since I originally asked that question only convinces me further that it is the right path. It is such a pain to work with currently due to the combination of build times and junk output. Don't get me wrong, I understand the value and virtue of a small output but trying to save a rather insignificant amount of bytes for a small minority of users just seems silly. The efforts are totally lost extremely quickly. Only noticeable if someone really cares, and if they care that much, they'd probably be using a build tool that can tree-shake.

I thought I'd bring this up again as I was reading through Microbundle's and some other bundlers' documentation trying to find an easy solution but I couldn't help but think back to this.

[omgovich]

Hey @ryanchristian4427. I was also thinking about that recently and moving to tree-shakable named exports seems the right decision. So we are on the same page here)

I think we should consider it as our v3 since those are breaking changes.