bgrins / TinyColor

Fast, small color manipulation and conversion for JavaScript
https://bgrins.github.io/TinyColor/
MIT License
5.08k stars 438 forks source link

fix: make TinyColor.random property static #177

Closed scttcper closed 6 years ago

scttcper commented 6 years ago

having a weird night with git, ignore that other pr.

bgrins commented 6 years ago

Looks good, thanks!

scttcper commented 6 years ago

if we're looking to support https://github.com/bgrins/TinyColor#random

Its already supported without it being a static member of the class. see the console in https://typectrl.github.io/tinycolor/ should have it now too.

const tinycolor = require('tinycolor2')
tinycolor.random()

what this adds is

import { TinyColor } from 'tinycolor2'
TinyColor.random();

which i could see also being desirable. Actually, now that i've written it out. I could see why you want it in there. 👍

bgrins commented 6 years ago

Yeah. Once other nice thing about it is that it should allow us to export the class directly for UMD so when loading via <script> tag we won't have to expose both tinycolor and tinycolor.TinyColor (there could just be the TinyColor class. When loading via node or other loaders it may be niceto be able to pull in random and other statics directly from the module, but when loading through a script tag I think it's nicer to reference them as statics on a class instead of having to dig through the export to get to the class.

bgrins commented 6 years ago

When loading via node or other loaders it may be niceto be able to pull in random and other statics directly from the module

We could consider dropping that entirely, and only ever export the class (with statics attached). I'd be curious your opinion on that.

scttcper commented 6 years ago

right now random and the others can be imported with

import { random, readability } from 'tinycolor2';
// or 
const random = require('tinycolor2').random;
random()
// or
const TinyColor = require('tinycolor2').TinyColor
TinyColor.random()

I'm not sure how statics affect tree shaking. Meaning if I use TinyColor, but I don't use or want random, if it will still be included in my final bundle when using rollup or webpack.

So i'm in favor of keeping as much as possible out of the class because with web tools as they are today, the bundle can be kept smaller if the classes are smaller.

bgrins commented 6 years ago

I'm not sure how statics affect tree shaking. Meaning if I use TinyColor, but I don't use or want random, if it will still be included in my final bundle when using rollup or webpack.'

Interesting - yeah I'm guessing they will have to be included in that case. What does that do to the budle size if we add all of the statics on the class instead of as exports?