graypegg / chromatism

:rainbow: A simple set of utility functions for colours.
1.78k stars 37 forks source link

Convert CJS modules into ESM #18

Closed mpalmr closed 7 years ago

mpalmr commented 7 years ago

I'm wondering if there's a better way to export things in /src/entry.js? Are there any tree shaking concerns with this?

graypegg commented 7 years ago

Sorry I don't have much time to look thru it right now, but as far as I can tell, entry will have to export unmodified return values from modules in order to properly tree shake, which means we can't use the same object for constants + operations like before. Roll-up+babel should also be able to convert a fully-es6-module-ed package to umd for release so we can use export in entry.js.

Saying all of that, doesn't look like constants are being exported ATM, thinking they'll just be part of the export like:

export { operations, constants }

Hopefully that made some sense!

TehShrike commented 7 years ago

If the constants are worth exposing via some API, I think it would probably be better to move them to their own module that people can import directly if they want to access. My preference would be for this module to have a simpler API - only exposing small, pure functions.

TehShrike commented 7 years ago

It's happening!

graypegg commented 7 years ago

@TehShrike it's worth exporting the illuminants at least, as they can be used for chromatic adaptation, though I'll admit, it's not a perticularally useful function. Otherwise, transform matricies aren't useful at all in normal use, maybe it's worth exporting the getIlluminant() helper as a operation?

TehShrike commented 7 years ago

I try to be very conservative when it comes to expanding APIs - how about publishing this breaking version without exposing those functions, allowing us to add them later in a feature version if someone has a need for them?

graypegg commented 7 years ago

@TehShrike Without exposing the adapt() function, or the illuminants, or both? The illuminants are the colours that adapt() uses to match whitepoints, I could rely on the user to supply their own, but then it'd require them to get accurate values for the whitepoint they want to adapt to, which I don't think is ideal. I could adjust adapt() to take a string like "D65" or "F2", and just pass that to getIlluminant() to get the adaptation whitepoint. That way, we don't have to include any constants in the API. Thoughts?

TehShrike commented 7 years ago

It makes sense to me to be able to take in illuminants as strings, as long as it's reasonable to assume that people will only be using those constant values. Which seems to be the current assumption, since the current readme only mentions the constant names and doesn't describe the data structure that you would pass in if you weren't using a predefined value.

graypegg commented 7 years ago

Anything else would be pretty odd, so I think I'm good with that too. I'll get adapt() set up that way when I'm back home from holiday!

mpalmr commented 7 years ago

I'm thinking any changes to the API period should be in a major version release even if it's not a breaking change? Exposing the constants isn't too crazy though so I have no strong opinions on that, or this in general.

TehShrike commented 7 years ago

This refactor/tiny-functions branch will most definitely a breaking API change, since my previous refactors completely changed the color objects that all these functions return, making them no longer chainable.

graypegg commented 7 years ago

Sorry, still a bit jet lagged! I just need to fix up the adapt() function ( #19 ) then I think we'll be good for a major version bump. I'll also update the README in a bit, shouldn't be THAT much to change.

Should also mention, chromatism will hopefully be added to CDNjs soon, so we should make sure it can export as a global. I might need to put together another bundle? (UMD package like now? Still need to play around with this branch more)