search

sass / embedded-host-node

A Node.js library that will communicate with Embedded Dart Sass using the Embedded Sass protocol
MIT License
144 stars 26 forks source link

[Color 4] Add support for CSS Color Level 4. #259

Closed jgerigmeyer closed 10 months ago

jgerigmeyer commented 11 months ago

Spec - https://github.com/sass/sass/blob/main/proposal/color-4-new-spaces-js.d.ts.md

ntkme commented 11 months ago

This faces a challenge like Calculation simplification, which we decided to not implement on the host side. Currently what I see from this PR is that we would need to implement the same feature twice, once in dart for dart-sass, and then once in pure js for this project. I think we should consider adding Color.change feature to embedded protocol, that is to let the host send an existing color and arguments to change, and then the embedded compiler returns the color after change. This way we don’t need implement the same logic twice or worry about potential drifts in different implementation.

nex3 commented 11 months ago

@ntkme I'm generally wary about asking embedded hosts to make round trips with the compiler just to do value operations, because it opens up a lot of difficulties both in terms of the API design (especially in languages with a strict distinction between synchrony and asynchrony) and in terms of performance (I'd like to avoid hidden cliffs where an operation that looks like it could be efficient is actually very slow). I think maybe the best solution here would just be to say that we don't mandate embedded hosts support all the color operations that the JS API does.

jgerigmeyer commented 10 months ago

@nex3 I think this could use another round of quick review -- thanks!

jgerigmeyer commented 10 months ago

@nex3 I think this is ready for another round of review. AFAICT all of the API is implemented. I can't reproduce the failing tests locally -- something to do with the legacy color definition in the embedded host? CI is passing.

jgerigmeyer commented 10 months ago

@nex3 Thanks for the review -- I think I've addressed everything, and this is ready for another round!

nex3 commented 10 months ago

Can you rebase this onto the feature.color-4 branch?

jgerigmeyer commented 10 months ago

Can you rebase this onto the feature.color-4 branch?

@nex3 Done, though I had already been basing this on main and it looks like feature.color-4 is behind main, so there are some additional changes here until main is merged into feature.color-4.