Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Color spaces are not tree-shakeable #163

Closed
LeaVerou opened this issue Jun 28, 2022 · 9 comments
Closed

Color spaces are not tree-shakeable #163

LeaVerou opened this issue Jun 28, 2022 · 9 comments

Comments

@LeaVerou
Copy link
Member

(Remaining part of #159)

While the new procedural API is tree-shakeable wrt functions, since color spaces register themselves onto ColorSpace.registry, this means they are not tree-shakeable.

To make them tree-shakeable, we'd need to eliminate side effects from the color space modules. Then, the importing code would register them.

Something like:

import ColorSpace from "COLORJS_ROOT/src/space.js";
import oklch from "COLORJS_ROOT/src/spaces/oklch.js";
ColorSpace.register(oklch);

We could make this less painful by offering pre-generated indices that do this for all existing color spaces, but I'm still struggling with the idea of requiring more plumbing to use Color.js, which makes it easier to make mistakes. E.g. someone could easily forget to register their color space and do something like:

import oklch from "COLORJS_ROOT/src/spaces/oklch.js";
let color = new Color("oklch", [.5, .2, 340]); // TypeError: No color space found with id = "oklch"

Note that registration is needed only to use string ids instead of color space objects. The API is perfectly usable with just passing identifiers around and never registering any color space, like so:

import srgb from "COLORJS_ROOT/src/spaces/srgb.js";
import oklch from "COLORJS_ROOT/src/spaces/oklch.js";
let color = new Color(oklch, [.5, .2, 340]);
color.to(srgb);

Thoughts?

@sgomes
Copy link
Collaborator

sgomes commented Jun 29, 2022

Thank you for spending so much effort working on a procedural API 🙇 That massively helps with keeping bundle sizes down!

Note that registration is needed only to use string ids instead of color space objects.

Yes, that's what I see as the greatest difficulty in using the current color space approach for everything. Would it be feasible to make parsing and serialization self-sufficient within each colour space?

The way the top-level parse and serialize functions are generated is already really modular, so it may be a case of making a couple of utils that are just a bit more generic than that, and using them in every colour space to generate standalone versions there?

@LeaVerou
Copy link
Member Author

serialize should already work without registered color spaces, it just looks up what formats are available in the color space of the object passed. parse is harder though, as until you parse, you don't know what color space you have. Perhaps it could accept an optional list of color spaces that will be used instead of ColorSpace.all / ColorSpace.registry?

@LeaVerou LeaVerou reopened this Jun 29, 2022
@sgomes
Copy link
Collaborator

sgomes commented Jun 30, 2022

Perhaps it could accept an optional list of color spaces that will be used instead of ColorSpace.all / ColorSpace.registry?

Yeah, an optional list of color spaces would likely do the trick here 👍 That would allow for use of the function while having an empty registry.

serialize doesn't need it, as you mention, but perhaps it could do the same, for symmetry? Though I'm really not sure. I can't decide whether it would indeed make things easier to use or just unnecessarily introduce more complexity 🙂

@LeaVerou
Copy link
Member Author

LeaVerou commented Jul 7, 2022

This should have been auto closed after #184 was merged, so I'm gonna manual close it.

@LeaVerou LeaVerou closed this as completed Jul 7, 2022
@lydell
Copy link

lydell commented Aug 15, 2022

FYI https://colorjs.io/docs/spaces.html says that this issue is open – should that be removed from the page?

image

@LeaVerou
Copy link
Member Author

Oops, yes! PR?

@lydell
Copy link

lydell commented Aug 15, 2022

PR?

I’m sorry, but I’ll pass on that this time! 😊

@LeaVerou
Copy link
Member Author

Can you open a new issue then since this is closed?

@lydell
Copy link

lydell commented Aug 15, 2022

Sure thing! Opened a new issue: #204

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants