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

Tree-shakeable, faster, module-based, procedural API #162

Merged
merged 5 commits into from
Jun 28, 2022
Merged

Conversation

LeaVerou
Copy link
Member

@LeaVerou LeaVerou commented Jun 27, 2022

Will close #159 once merged

The idea is:

  • Functions defined in modules.
  • Functions are standalone and can work on either plain objects (with the right properties) or actual Color objects.
  • Functions return plain objects
  • We only use these plain functions internally
  • color.js becomes optional
  • Plain functions added on Color with a simple method call and then they return Color objects instead of plain objects (staying compatible with the current API)
  • index.js imports everything, including Color
  • index-fn.js imports and re-exports all functions

This means there is both a tree-shakeable, procedural API that is faster and easier to debug, as well as a slower OO API that can be more readable in certain cases.

The new API resulted in a 4x speed increase in this benchmark inspired by @ai's use case.

@LeaVerou LeaVerou requested a review from svgeesus June 27, 2022 14:30
@LeaVerou LeaVerou changed the title Tree-shakeable, faster API Tree-shakeable, faster, module based procedural API Jun 27, 2022
@LeaVerou LeaVerou changed the title Tree-shakeable, faster, module based procedural API Tree-shakeable, faster, module-based, procedural API Jun 27, 2022
@LeaVerou LeaVerou marked this pull request as ready for review June 27, 2022 17:24
@LeaVerou
Copy link
Member Author

I still need to add docs but I think I fixed all the regressions and this should be ready for review.
@ai feel free to weigh in too (it won't let me add you as a reviewer).

in: color_srgb => color_srgb.inGamut(undefined, {epsilon: 0}),
str: color_srgb => color_srgb.toString()
create: (l, c, h) => ({space: oklch, coords: [l, c, h]}),
to_srgb: color => to(color, "srgb"),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does srgb color space register in to()?

We can do to(color, srgb) to avoid registration.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. It doesn't yield much of a difference performance-wise though.

import set from "./set.js";

export function lighten (color, amount = .25) {
let space = ColorSpace.get("oklch", "lch");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not very familiar with ColorSpace but should we import oklch color space in this file to be sure that color space was loaded?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ColorSpace.get("oklch", "lch") loads oklch if it's available, or lch if it's not, i.e. loading oklch progressively enhances how lighten() works.

src/index-fn.js Outdated
@@ -0,0 +1,14 @@
export {default as getColor} from "./getColor.js";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we replace export default to export in ./*.js this file will be smaller.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes but default exports are nicer for the individual modules.

Copy link
Member

@svgeesus svgeesus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one comment on need for documentation; no changes requested

color1 = to(color1, space);
color2 = to(color2, space);

// Gamut map to avoid areas of flat color
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should document that GM happens before interpolation here, especially as this is a change from CSS Color 4

@LeaVerou LeaVerou merged commit 1689874 into main Jun 28, 2022
@LeaVerou LeaVerou deleted the treeshakable branch June 28, 2022 12:06
@LeaVerou
Copy link
Member Author

Changes are in! Time to do a new release on npm and see how it goes.

@manuelmeister
Copy link
Contributor

Is it correct that the named contrast functions e.g. contrastAPCA are not included in colorjs.io/fn?

@LeaVerou
Copy link
Member Author

LeaVerou commented Nov 7, 2022

Is it correct that the named contrast functions e.g. contrastAPCA are not included in colorjs.io/fn?

They should be. If not, it's a bug. But I believe they are exported en masse from contrast/index-fn.js, so if you're searching the source for contrastAPCA it won't find anything.

@manuelmeister
Copy link
Contributor

Okay I'll pull the last version again and look at the typings again.

@MysteryBlokHed
Copy link
Member

There's a file src/contrast/index.js that exports all the contrast functions, but it doesn't seem to be re-exported in src/index-fn.js:

https://github.com/LeaVerou/color.js/blob/0b3cacae4af0b93fd0d21ff88b21efd3be27109d/src/index-fn.js#L1-L30

@MysteryBlokHed
Copy link
Member

Just opened #240 to add the missing export

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

Successfully merging this pull request may close these issues.

Tree-shaking ready API
5 participants