Large bundle size

[bradleyayers]

I'm a fan of this library, but I wish it was smaller. I think a good approach would be to refactor it a little into modules that can either be imported separated or tree-shaken. Tree-shaking is probably the simpler approach, given the multiple distribution formats (pkg.main, pkg.module). color.ts is the biggest culprit, with loads of named exports and having heaps of operations baked into ColorHelper.

I've personally found the RxJS packaging to be wonderful (as a consumer). I'm not proposing we move completely to that model, but I could imagine moving a little in that direction. For example removing named exports of each color.

Here's an example on the result:

8.5.0:

With named colors deleted:

With named colors moved to a separate module, with a single named export:

In this scenario the module lived at internal/namedcolors.js and looked like:

export var namedColors = {
  aliceblue: rgb(240, 248, 245),
  antiquewhite: rgb(250, 235, 215),
...

[notoriousb1t]

It is true that the named colors account for most of the size of the library when bundled. I think if we do move them out, we will need to move major version because it would break how color() parses.

Maybe it might be time to revisit the API and come up with a different, more modular way to structure it so that it minifies well.

I also have a bone to pick with having .toString(), toHexString() as well. I am thinking that restructuring colors with a pipeline api would be appropriate to solve both problems:

// outputs color as hex string
const primaryColor = color('#0000FF', darken(0.1, true), saturate(0.1), toHex);

// outputs color as rgba() string
const primaryHover = color(primaryColor, lighten(0.1), toRGBA);

// outputs a string
const secondaryColor = rgba(255, 0, 0, .1);

// outputs background color as string
const backgroundColor = color(fromNamedColor('white'), darken(.1, true), toRGBA);

// and also this would be possible through overloading.  it would output hex
const backgroundColor2 = darken(rgba(255, 255, 255, 1), .1, true);

I think this would result in only what is needed being included in the build without sacrificing the ability to compose new colors. The negative part is that it is a U-turn in api design and would effectively be a re-write of the colors.

On the other hand, I think that this would greatly improve the usability and size of the library..

What do you think @basarat?

[notoriousb1t]

To clarify, the overloads of darken would look like this:

export function darken(color: string, percent: number, relative?: boolean): string
export function darken(percent: number, relative?: boolean): { (color: number[]): number[] }

[bradleyayers]

This would definitely be backwards incompatible, so it'd be great to incorporate any other breaking changes at the same time!

I'm not convinced by the pipeline API proposal. For me personally it's unnecessary sugar, but I can appreciate that for others it'd be a delight.

I'm a fan of the approach https://date-fns.org/ has taken, where they provide simple utility functions that have limited scope, and leave composition up to the consumer. I like the approach because:

• it's optimised for bundle size, meaning the library is perfect for becoming the defacto solution for the low-level calculation
• it's a simple API contract to maintain and support, so library maintenance is easier
• it leaves the door open for other libraries to implement higher level abstractions (e.g. a pipeline API)

In regards to the following code, the bundle would need to incorporate the logic for both APIs, which I'd like to avoid in the pursuit of minimal bundle size.

export function darken(color: string, percent: number, relative?: boolean): string
export function darken(percent: number, relative?: boolean): (color: number[]) => string

An API like this would avoid that issue:

export function darken(color: Color, percent: number, relative?: boolean): Color;
export function toHex(color: Color): string;
export function toRGBA(color: Color): string;
export function fromNamed(color: string): Color;
export function fromHex(color: string): Color;

[notoriousb1t]

The reason I am against an API that formats at each step is that a subsequent parse (and possible color conversion) is required on the following step. I also don't think that would be good for chaining actions to be applied to a color. Consider the following real-word example with three functions nested:

const myColor = toHSLA(darken(rgb(0, 0, 255), .1, true))

1. After a single nested function, readability goes out the window. Because darken() has additional parameters, it can be difficult to determine at a glance which parameters are for what function. The user is forced to create a bunch of unnecessary local variables or they have to adopt a functional programming library with pipe/compose functions. I assume that the vast majority of developers are not familiar with these concepts based on my own experiences with others. That is not to say that compose/pipe functions aren't simple by nature or not readily available.

2. Darken is not capable of being partially applied. Being able to do partial applications of functions is something that has to be thought about in advance. The signature darken(color, percent, relative) is not conducive to this. The date-fns has the right idea in its fp functions. If we start by building the fp functions, the one-off string returning functions will simply be composed of them. To some degree, I think it might be possible to write a mixin function that will do this automatically.

3. Parse, Convert, Format, repeat. Without an obvious built-in way of doing FP programming, at each stage the function might need to parse the colors from a string, convert it to the proper color space to do its action, convert it back to the color space it came in as, and then format it).

In that example, the color would need to be parsed 3 times, converted to HSLA twice, converted to RGBA once, and then formatted 3 times with a simpler API. With a pipeline api that is aware of the colors, that would require parsing once, converting to HSLA once, and formatting once.

const myColor = color(rgb(0, 0, 255), darken(.1, true), toHSLA)

I realize we are talking about string manipulation, and creating numeric arrays, so it isn't absolutely going to cripple an application, but it isn't free and I personally use the color api a great deal. I think it is possible to do both while greatly improving the bundle and execution times.

The nice thing about having two ways of doing it is that even the same people program differently in different circumstances. That is the reason we have cssRaw(), cssRule(), and style() in TypeStyle.

[bradleyayers]

In my proposal the utility functions would take and return a Color rather than a formatted string. I'm also imaging conversion between color systems would be lazy. Conversion from a string to a Color would be explicit using separate functions.

With respect to the pipeline style API, I'm warming to the idea. I'm not convinced having two ways of doing something is better than one though.

So I'd be totally happy with this API:

export function darken(percent: number, relative?: boolean): (color: Color) => Color;
export function lighten(percent: number, relative?: boolean): (color: Color) => Color;

export function toHex(color: Color): string;
export function toRGBA(color: Color): string;
export function fromNamed(color: string): Color;
export function fromHex(color: string): Color;

export function chain<T>(...fns: (t: T) => T): (t: T) => T;

[notoriousb1t]

So if we are talking about a declarative format for Color:

interface Color {
    t: number
    c1: number
    c2: number
    c3: number
    a: number
}

Something like this would be appropriate. Property accesses are faster than array accesses and since property names normally are preserved during minification, using short names is appropriate. c1 -> c3 are rgb and hsl respectively. With a composable API, it doesn't need any functions, so it could even be an object literal.

I am on board for dropping to one API style if it is chainable. I am okay with that last API proposal with one exception:

I think for the sake of usability having hex/color function parsers together makes more sense than separating them into fromHex and from?. Having fromNamed separate make sense given this thread, but separating hex/colorfunctions puts more burden on the user for little gain in minified size or performance.

So going back to my earlier examples, with this API style:

const myColor = chain(rgb(0, 0, 255), darken(.1, true), toHSLA)

const myColor = toHSLA(darken(.1, true)(rgb(0, 0, 255))

The type for the chain function will not be so easily declared, however. In order for the types to automatically be detected it will probably have to be something like this:

export function chain<T1>(t: T1): T
export function chain<T1, T2>(t1: T1, t2: (t: T1) => T2): T2
export function chain<T1, T2, T3>(t1: T1, t2: (t: T1) => T2, t3: (t: T2) => T3): T3
... and so on ...

The function's implementation would be simple, but I can't imagine another way to handle that such that the output parameter is automatically inferred. Maybe there is another way to do that in TypeScript that I don't know about shrug

[bradleyayers]

Another approach worth considering is what RxJS does, where you can add operators to the prototype of Observable by importing something like import 'rxjs/add/operator/map'.

Check out https://github.com/ReactiveX/rxjs/blob/master/src/add/operator/map.ts for an example.

They package both individual operators (e.g. https://github.com/ReactiveX/rxjs/blob/master/src/operator/map.ts) as well as the 'modify the prototype' methods. I've found it to be quiet convenient.

I think this approach would basically be my original proposal, but would offer chaining via methods directly on Color.

[notoriousb1t]

Sleeping on the idea has made me more amenable to simply returning strings because in best practice, you should't be using the color functions enough for it to matter too much. Since it is much easier for the user to use string returning functions, that might outweigh the optimizations of creating chainable APIs, and in reality, a function like darken could just be nested in a lambda .map((s: string) => darken(s, .1))

I think for now it is a good first step to break apart ColorHelper into its respective pieces using a similar methodology to RxJs. I think I can do that in a backward compatible way for now and then once it is all separated, we can figure out what we should do to improve it.

[bradleyayers]

The problem I see with just accepting and returning strings is that it means named colors can't be supported as one of the valid "string" values (whilst still being excluded from the core bundle).

On a more philosophical level, I'm personally not a fan of using a string interface between operators because:

• it's a much looser contract than a TypeScript object/class interface
• it's arguably more ambiguous regarding the returned value (e.g. is it hex or RGBA or HSLA or something else?)

For me I can't imagine the performance impact (execution time) of a parse/serialise between each operator being significant enough to have much weight in the API design.

[notoriousb1t]

The problem I see with just accepting and returning strings is that it means named colors can't be supported as one of the valid "string" values (whilst still being excluded from the core bundle).

I think this can be accomplished by making color() only handle hex and color functions and fromNamedColor() handle named functions. If you never use fromNamedColor and we have separate imports, you would not need the full set of definitions.

Personally, I rarely use actual color names and the browsers return colors in rgb, so I don't see it as a huge issue in making it a tiny bit less convenient to use parse color names. I am speaking only for myself though, maybe some people use color keywords frequently? This is what I am suggesting in that case:

import { white } from 'csx/color-names';

import { fromNamedColor } from 'csx/color-names';
const white = fromNamedColor('white') // rgb(255, 255, 255)

import { color, darken } from 'csx/colors';
const white = '#FFFFFF'
const gray = darken(white, .5) // #808080

All of those would return a CSSColor (a string) and would keep the same color space and format unless it is a conversion function.

But in any case, I am going to branch and start getting the split prototype thing going tonight since it should be backwards compatible.

[bradleyayers]

Sounds good to me! Keen to see what you come up with :)

[notoriousb1t]

Ok, I have a working build with everything separated out. It isn't quite as separated as RxJs at this point with both functional and fluent styles, but it is a step in the right direction: https://github.com/typestyle/csx/tree/colors-split-prototype

The main index file for colors now does an import. Importing names has to come last because they depend on rgb/rgba:

// core imports
export { ColorHelper, color } from './color-helper'

// formatters
import './to-string'
import './to-hex-string'

// ColorHelper optional functions
import './darken'
import './desaturate'
import './fade-in'
import './fade-out'
import './fade' 
import './grayscale' 
import './invert'
import './lighten'
import './mix'
import './saturate'
import './shade'
import './spin'
import './tint'

// parsers
import './parse-color-function'
import './parse-hex-code'

// parse-named-color requires ALL named colors, increasing build size significantly
import './parse-named-color'

// export color creators
export { hsl } from './hsl'
export { hsla } from './hsla'
export { rgb } from './rgb'
export { rgba } from './rgba'

// color keywords
export * from './names'

Each file has a module declaration and assigns the function to the colorPrototype:

import { ColorHelper, colorPrototype, createColor, convert } from './color-helper'
import { HSL, L, maxChannelValues } from './constants'
import { ensurePercent } from '../../utils/formatting'

export function darken(this: ColorHelper, percent: string | number, relative?: boolean): ColorHelper {
    const current = this
    const v = current.toHSL()
    const l = v.c3 - (relative ? v.c3 : maxChannelValues[HSL][L]) * ensurePercent(percent)
    return convert(createColor(HSL, v.c1, v.c2, l, v.a, current.isAlpha), current.s, current.isAlpha)
}

colorPrototype.darken = darken

declare module './color-helper' {
    interface ColorHelper {
        darken: typeof darken
    }
}

And each color parser including parseNamedColor adds itself to the list of color Parsers used by the color() function:

import { ColorHelper, colorParsers, createColor } from './color-helper'
import { RGB } from './constants' 

/**
 * 
 * @param stringValue 
 */
export function parseHexCode(stringValue: string): ColorHelper | undefined {
    const match = stringValue.match(/#(([a-f0-9]{6})|([a-f0-9]{3}))$/i)
    if (!match) {
        return undefined
    }

    const hex = match[1]
    const hexColor = parseInt(hex.length === 3 ? hex[0] + hex[0] + hex[1] + hex[1] + hex[2] + hex[2] : hex, 16)
    const r = (hexColor >> 16) & 0xff
    const b = (hexColor >> 8) & 0xff
    const g = hexColor & 0xff

    return createColor(RGB, r, b, g, 1, false)
}

// add to parsers
colorParsers.push(parseHexCode)

I think this methodology will largely work. It isn't 100% backward compatible from a TypeScript perspective because Color Helper is an interface. If we want to ship this type of functionality, I think we need to reorganize the library such that there is an obvious and nice looking way to use the other functions in other sections without having to do an import from csx directly. Importing csx directly for any reason will still touch that color/index script and import everything. Thoughts?

[bradleyayers]

Looks great!

Importing csx directly for any reason will still touch that color/index script and import everything

I think this is fine. I think internally I already have that expectation — that is if I import from a package, I'm potentially getting everything.

I think we need to reorganize the library such that there is an obvious and nice looking way to use the other functions in other sections without having to do an import from csx directly.

Are you referring to the situation for library consumers, or for code within the library itself? As a consumer I'd like to be able to do something like import darken from 'csx/operator/darken'; (I think default exports are probably a little nicer here for single-function-modules than named exports).

This would also require a bit of adjustment to the way the built files organised, as the hierarchy must be placed in the root of the package, as it can't use pkg.main.

What are the next steps in your mind?

[notoriousb1t]

Importing csx directly for any reason will still touch that color/index script and import everything

If we go forward with selective imports, we will need to reorganize the folder structure such that a consumer can import functions directly rather than from "csx" directly. I think we should allow export from "csx" for backward compatibility. The short of it is that the API needs to be consistent with itself, so we can't just do this for colors.

I think default exports are probably a little nicer here for single-function-modules than named exports

I am 100% against default imports because it locks the API into only exporting one thing. There are a few other reasons as well. I think @basarat puts it pretty well in his deep dive book:

https://basarat.gitbooks.io/typescript/content/docs/tips/defaultIsBad.html

This would also require a bit of adjustment to the way the built files organised, as the hierarchy must be placed in the root of the package, as it can't use pkg.main.

I don't have an issue with moving everything up from internal. We named it internal as a way to signify that it was off-limits for direct importing. This move is counter to that.

What are the next steps in your mind?

1. I would like a little more feedback from members of the org for check and balance reasons.
2. I think we should figure out the rules for the new folder structure. Initial thoughts:

// import color directly (limits side-effects)
import { color } from 'csx/color'

// add darken to color prototype
import 'csx/color/add/darken'

// import darken as a standalone function
// not 100% on this, but it would certainly be easy to use.  
// This was the string -> string api I was describing earlier
import { darken } from 'csx/darken'
const darkenedColorString = darken(colorVar)

// import with everything (backward compatible)
import { color, darken } from 'csx'

And then transform would need to be something like this:

// separate imports
import { transform } from 'csx/transform'
import { scale } from 'csx/scale'
import { rotate } from 'csx/rotate'
import { translateX } from 'csx/translateX'
const value = transform(rotate(-90), scale(1.3), translateX(-20)) 

// from csx for backward compatibility
import { transform, scale, rotate, translateX } from 'csx' 
const value = transform(rotate(-90), scale(1.3), translateX(-20)) 

3. We need to go back to the earlier conversation and determine if having a flat API in addition to the color helper is worth it. I think that it brings a lot of convenience and would improve readability in simple cases, but it would certainly increase the surface area of the API.

4. I think we either need a dedicated section with index in the TypeStyle docs or separate documentation. Even if there are no breaking changes, the import system requires additional explanation. I don't really think that should be nested inside the TypeStyle docs. At present there is undocumented functionality due to not fitting in the current doc's structure. I haven't documented many of the helpers because it would bloat TypeStyle's docs.

[bradleyayers]

I'm on-board with everything you said 👍.

[Frikki]

I am 100% against default imports because it locks the API into only exporting one thing.

I agree completely.

Ad 2) Looks promising.

Ad 3) I find that smaller APIs tend to be more successful. Naming conflicts are also easier avoided, both internally and externally.

Ad 4) Yes. A dedicated documentation section would be nice.

[notoriousb1t]

Closing as this was addressed through removing named colors in 9.0.0