umd exports

[scttcper]

here's what this looks like when loaded in browser https://jsbin.com/noqufepofi/edit?html,js,console

keeps all compatibility for current CDN users. People using node or js with webpack/rollup will get the advantage of a lighter bundle by keeping the static methods out of the TinyColor class.

cdn users get what they expect, webpack users get what they want.

[codecov-io]

Codecov Report

Merging #178 into v2 will increase coverage by 0.18%. The diff coverage is 100%.

@@            Coverage Diff            @@
##              v2     #178      +/-   ##
=========================================
+ Coverage   98.4%   98.59%   +0.18%     
=========================================
  Files         10       10              
  Lines        628      640      +12     
  Branches     150      150              
=========================================
+ Hits         618      631      +13     
+ Misses        10        9       -1

Impacted Files	Coverage Δ
src/index.ts	97.85% <100%> (+0.11%)	:arrow_up:
src/random.ts	97.27% <0%> (+0.9%)	:arrow_up:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update d6d8e04...b4cd241. Read the comment docs.

[bgrins]

Thanks! It looks like this gives the v1 backwards compat as we were discussing. I'm not sure about supporting two different APIs for node vs umd, though. Two questions:

1) I'm assuming that the tree shaking wouldn't work if we exported this in the public API, right? 2) If (1) is true, then how much code size do we actually save in node / webpack by exporting the static functions separately instead of on the class? I think these are all relatively small utilities so am unsure that it's worth breaking backwards-compat to provide the code-size reduction.

[scttcper]

Yeah treeshaking pretty much only works when exporting individual classes or functions and then determining that they don't depend on each other which it can't determine if we mash them into a single expore like the UMD. I think the api's are similar enough to where it shouldn't be much of a pain?

as in this works the same in both

const tinycolor = require('tinycolor2')
tinycolor('red')
tinycolor.random()
new tinycolor.TinyColor('black')

using only the TinyColor class is 4.64 kB gzipped using tinycolor.random() (which returns TinyColor class) is 5.65 kB gzipped exporting both the same as the UMD its 6.22 kB no matter what is used

I guess if webpack/node users really care they can specify the file directly and that will also be tree shakeable.

// es import
import { random } from 'tinycolor2/es/random'
// import from cjs
const random = require('tinycolor2/random').random

Let me know what you think, I can change us over to only use the UMD style export no problem.

[bgrins]

as in this works the same in both

const tinycolor = require('tinycolor2')
tinycolor('red')
tinycolor.random()
new tinycolor.TinyColor('black')

Oh interesting, I wasn't expecting that this would work from the public API as well. If that's the case, is there any difference at all between public_api and umd_api? And, could we export the public API directly for UMD but change some options (exports: 'default' maybe) in order to get the same effect as adding the new umd_api?

[bgrins]

Playing around with this locally, we can't just do exports: 'default' on the public API for UMD build. AFAICT if we want to do the same (v1) API for both cases, the simplest way would be to export each helper method as a static on the TinyColor class in index.ts, and then export the class directly as a default export (for both public and umd api). The downside of this approach is that we'd lose tree shaking, but given the sizes we are talking about here that doesn't seem like a dealbreaker (at least to me).

[scttcper]

Yeah using exports default doesn't match how its exported in the current version, played with that as well.

Adding them as static methods adds a bunch of circular dependencies that will throw warnings for the webpack/rollup crowd unless its brought into the same file and of course the tree shaking of a few kb.

(!) Circular dependency: dist/index.js -> dist/random.js -> dist/index.js

Is going with the "UMD" export for everything an option? You liked it earlier for just the umd/cdn version. I think it does pretty good at covering all the bases. I've adjusted this pr to what that would look like.

[scttcper]

shoot... it breaks a bunch for the node users. uhhhhh i'll make a pr to do it the static way

[scttcper]

Alright i've added the static methods, but had to keep the umd_api.ts since it only allows the one default export.

If we remove the named exports in public_api, the tests and readme will also have to be changed to the following as random cannot be imported directly.

import * as tinycolor from 'tinycolor2'
tinycolor.random();

typescript users will have to call tinycolor as a function first. Edit: just tried it and typescript errors completely.

import * as tinycolor from 'tinycolor2'
tinycolor().random()

I think i preferred when the umd users got what they wanted and the node users got what they wanted from earlier. If we export just the class as the default export, we break backwards compatibility of calling TinyColor without new.

[bgrins]

I think i preferred when the umd users got what they wanted and the node users got what they wanted from earlier. If we export just the class as the default export, we break backwards compatibility of calling TinyColor without new.

Huh, I wasn't expecting that. OK, I'll pull this down today and play with it.

[bgrins]

I did some work with this locally and I think I'm seeing why exporting the wrapper function rather than the class causes some issues here. I think we'll either need to attach fromRatio, isReadable, etc onto the wrapper function, or drop the wrapper function and force calling with new and attach those methods as statics on the class. I'm leaning towards the latter since it seems like the more well-worn path with TypeScript exports.

Going to spend a bit more time to make sure I fully understand what's going on.

[bgrins]

Continuing to play with this at https://github.com/bgrins/TinyColor/commit/e881ac8e13bb67c0e2c31803456d38bc255aa4ca.

As far as supporting the call without new, adding this to the constructor actually works (confirmed in UMD bundle):

    if(!(this instanceof TinyColor)) {
      return new TinyColor(color, opts);
    }

TypeScript just doesn't like it (so the test fails with "TypeError: Class constructor TinyColor cannot be invoked without 'new'").

[bgrins]

Closing this, as the commits landed in #181