fix: prerelease version compare, non-integer avatar box, HEAD for reachability - #76
Merged
Merged
Conversation
toSegments() split on "." without first dropping a "-rc.1"/"+build5"
suffix, so parseInt's usual truncate-at-the-first-non-digit-character
behavior ("0-rc" -> 0) left the suffix's own numeric parts as extra,
uncompared segments. "12.0.0-rc.1" parsed as [12, 0, 0, 1] against
"12.0.0"'s [12, 0, 0], so isNewerVersion('12.0.0', '12.0.0-rc.1') came back
true — a prerelease read as newer than the release it precedes, backwards
from real semver precedence, and the reverse comparison flagged the release
itself as not up to date even when it just shipped.
Fixed by dropping everything from the first "-"/"+" before segmenting.
This can't tell a prerelease apart from its release anymore (both parse to
the same core), but every real caller (npm dist-tags, Twemoji/Google Fonts
release tags) is a plain core version anyway, and comparing bare cores can
only ever call them equal — never flip which one is "newer".
Added version.test.ts; isNewerVersion had no dedicated tests despite package
update, Twemoji update and font update checks all depending on it.
box comes straight from ratio-based layout math (widthRatio * canvas width, etc.) and is rarely integer-aligned, but getImageData/putImageData need integer x/y/width/height. Passed through as-is, the fractional edge of the region could be clipped or misaligned in the ctx.filter-unsupported grayscale fallback, depending on how the binding coerces the values. Floors the origin and ceils the far edge instead, so the integer rectangle is always a superset of the original fractional one — the desaturated region can end up a fraction of a pixel larger than painted, never smaller. Added a test exercising this fallback path directly (drawAvatar had no tests at all): forced supportsFilter() to false via a new test seam and checked a pixel at the box's fractional far edge, which failed before this fix and passes after it.
checkReachable() only cares whether a request completes at all — even an error response proves the network and TLS are fine, per its own doc comment — so it never needed the body a GET downloads. registry.npmjs.org's `/makeitaquote/latest` in particular returns a full package manifest just to confirm the registry is reachable. Adds a head() method to HttpClient (identical to get(), just a different method) rather than a special case: a non-2xx or even a 405 for HEAD itself still proves reachability under this function's own definition, so no per-host allowance is needed for a server that doesn't implement HEAD well.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this change?
Three independent correctness fixes found during a codebase review, each its own commit:
isNewerVersion: a prerelease (12.0.0-rc.1) compared as newer than the release it precedes (12.0.0), backwards from real semver precedence —parseInttruncated"0-rc"down to its digits but still counted the suffix's own numeric parts (.1) as a real extra segment. Now drops everything from the first-/+before segmenting. Addedversion.test.ts— this had no dedicated tests despite package/Twemoji/font update checks all depending on it.desaturateRegion: rounds its box outward to integer pixels beforegetImageData/putImageData, instead of passing the ratio-based (rarely integer) layout box straight through. Added a test fordrawAvatar's grayscale fallback path, which had no coverage at all — verified it fails without the fix and passes with it.env's reachability check: usesHEADinstead ofGET— it only cares whether a request completes at all (even an error response proves the network/TLS are fine, per its own doc comment), so it never needed the body. AddedHttpClient.head().Checklist
npm run cipasses (Biome lint and format)npm run typecheckpassesnpm run testpassesnpm run build && npm run check:buildpasses