Repository navigation
ESM-only - #4542
ESM-only#4542florian-lefebvre wants to merge 18 commits into
Conversation
|
Tests pass locally for me. @lovell can you run the workflows? |
|
@lovell I think I fixed everything (coverage and node 20), should be good to go |
|
I just tried running the failing tests locally on Windows 11 and can't replicate, so I think that they should pass now that lint is fixed, but no guarantee :/ |
|
I think the issue was around the font file so I tried another approach of having the font file directly in the repo. I tried it locally on windows 11 and it worked but again, no guarantee. If that doesn't work, I'm out of ideas |
|
It worked 🎉 |
| @@ -4,7 +4,7 @@ | |||
| */ | |||
|
|
|||
| import color from '@img/colour'; | |||
There was a problem hiding this comment.
@img/colour is essentially the CommonJS version of the color package:
This package converts the color package and its dependencies, all of which are MIT-licensed, to CommonJS.
Since sharp is now ESM-only, this seems like a good time to migrate to color directly.
There was a problem hiding this comment.
I'll let Lovell decide but I think it makes sense
There was a problem hiding this comment.
For context I also opened e18e/module-replacements#786 about it
There was a problem hiding this comment.
i prefer not to self-promote but it seems relevant here: i did also launch hueful recently. which is more modern and faster than color.
Lovell, if you'd be open to trying it out in sharp in a follow up to this, i'd be very happy to help out. it'd drop a few dependencies too
There was a problem hiding this comment.
@43081j It gets my vote purely on the correct spelling of colour. It is able to parse objects like the following?
{ r: 255, g: 102, b: 0, alpha: 0.5 }
{ c: 1, m: 239, y: 227, k: 5 }
There was a problem hiding this comment.
As an aside, the @img/colour CJS wrapper is currently downloaded more than the upstream ESM-only color package 😅
There was a problem hiding this comment.
most of it operates on a [c0, c1, c2, alpha?] tuple.
we're missing the color() function but i plan on adding those too (which will bring in CMYK and some others).
i can make a draft PR to prove it out if you like and we can see how it goes that way
There was a problem hiding this comment.
@43081j Great, thank you. I prefer your space+tuple structure as it removes ambiguity e.g. the h property of HSL is not the same as the h property of OKLCH. It would be a breaking change, plus I guess ESM-only dependent.
|
We could also replace |
|
Looks like conflicts need to be resolved |
|
Yeah I resolve conflicts from time to time, I don't do it too often since there's no ETA |
Resolved conflicts so the ESM-only layout survives main's changes. - lib/index.mjs and scripts/build.mjs: deleted on the branch and modified on main. Kept the deletions and carried main's SPDX copyright header onto the renamed lib/index.js. - .github/CONTRIBUTING.md: kept main's rewrite and dropped the build:dist instructions for the CJS build the branch removes. - .github/workflows/ci.yml: kept main's Ubuntu 26.04 and action version bumps, kept npx patch-package, dropped the build:dist steps, and dropped build:dist from the new build-asan job. Kept the branch's Node.js version pins. - test/unit/tile.js: combined main's yauzl-based extractor with the branch's ESM imports.
This branch has not been deployed
5 participants
Closes #4539