[repro] Fix resolveFit contain/scale-down padding regression (upstream #14714)#2
Open
allocsys wants to merge 57 commits into
Open
[repro] Fix resolveFit contain/scale-down padding regression (upstream #14714)#2allocsys wants to merge 57 commits into
allocsys wants to merge 57 commits into
Conversation
…path Previously runTransform() hardcoded sharp's fit: "contain" (which letterboxes with black bars) for every resize, ignoring any fit, gravity, or background the caller passed on the transform. Wire up the same resolveFit/resolveGravity helpers already used for the cf.image fetch path, and forward background (defaulting to white for "pad", matching production). Fixes cloudflare#14695
Co-authored-by: devin-ai-integration[bot] <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
…dflare configuration (cloudflare#14534)
…loudflare/src/frameworks (cloudflare#14759) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Wrangler automated PR updater <wrangler@cloudflare.com>
…ace paths (Windows) (cloudflare#14713) Co-authored-by: dumbCodesOnly <225476909+dumbCodesOnly@users.noreply.github.com> Co-authored-by: Pete Bacon Darwin <pbacondarwin@cloudflare.com> Co-authored-by: Pete Bacon Darwin <pete@bacondarwin.com>
Co-authored-by: devin-ai-integration[bot] <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…lity inputs in `unstable_startWorker` (cloudflare#14617)
…flare#14448) Co-authored-by: Dario Piotrowicz <dario@cloudflare.com>
…/frameworks (cloudflare#14760) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Wrangler automated PR updater <wrangler@cloudflare.com>
…lare#14688) Co-authored-by: devin-ai-integration[bot] <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…andlers (cloudflare#14763) Co-authored-by: Edmund Hung <edmund@cloudflare.com>
…ing (cloudflare#14746) Signed-off-by: Sam Agarwal <samarthagrawal526@gmail.com>
Co-authored-by: Ben <4991309+NuroDev@users.noreply.github.com> Co-authored-by: Ben Dixon <ben@nuro.dev>
…cloudflare#14773) Co-authored-by: Ben <4991309+NuroDev@users.noreply.github.com>
Co-authored-by: James Opstad <13586373+jamesopstad@users.noreply.github.com>
…-cloudflare/src/frameworks (cloudflare#14659) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Wrangler automated PR updater <wrangler@cloudflare.com>
…dates (cloudflare#14715) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Wrangler automated PR updater <wrangler@cloudflare.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
…contain"), not "inside" Fixes image snapshot regression (expected 128x100, got 56x100) in get-platform-proxy Image bindings test. Cloudflare Images "contain" and the default "scale-down" fit both pad to the exact requested box with letterboxing, matching Sharp's "contain" fit - not "inside", which only constrains within the box while preserving aspect ratio without padding.
The previous fix conflated two different APIs that share this file: - Images binding (env.IMAGES.transform()): explicit fit:"contain" pads to the exact box (letterbox), but default/unspecified fit and "scale-down" must NOT pad - confirmed by transform.spec.ts. - cf.image (fetch cf.image option): fit:"contain" does NOT pad, per cf-image.spec.ts "fit:contain preserves aspect ratio". Introduces resolveImagesBindingFit() (contain -> pad, default/scale-down -> inside/no pad) used only by runTransform(), and keeps the original resolveFit() (contain -> inside, default/scale-down -> inside) for applyCfImageTransforms(), instead of one shared function.
…one line (verified with oxfmt locally against .oxfmtrc.jsonc)
…m-proxy Image bindings snapshot and upload it as an artifact for manual review
, c7b3219) to this branch's copy: (1) run through turbo/test:ci instead of calling vitest directly so wrangler's dist gets built first, (2) add --update flag so jest-image-snapshot actually writes a new baseline despite CI detection via is-ci/GITHUB_ACTIONS.
…ded, matches production-correct default/scale-down fit behavior after resolveFit/resolveImagesBindingFit split)
… runTransform() passed the raw Sharp Duplex stream directly into `new Response(transformer, ...)`, unlike cfImageLocalFetcher which explicitly buffers via .toBuffer() first. This pre-existing pattern (present before this PR's changes) apparently produced incomplete/corrupted output in some environments, only exposed by this PR's own new pixel-level regression tests (transform.spec.ts), which failed with Sharp "Input buffer contains unsupported image format" when re-parsing the response. Buffering explicitly, matching the already-working cf.image code path, fixes it.
Snapshot has been regenerated and committed; this throwaway workflow_dispatch job is no longer needed and was blocking Validate PR Description (new workflow files require justification).
All 6 tests were failing with "Input buffer contains unsupported image format" when decoding the transform response - the response body was actually a plain-text binding error, not image bytes. Passing the raw request.body ReadableStream into input() inside a dispatchFetch-invoked worker doesn't round-trip correctly through the local Images binding loopback; buffering to an ArrayBuffer first (as done implicitly by the passing getPlatformProxy-based fixture test, which reads via RPC rather than a live stream) avoids this.
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
20 participants
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.
Repro/root-cause PR for cloudflare#14714 on our fork, to verify the resolveFit() fix (commit a06fc78) resolves the failing get-platform-proxy Image bindings snapshot test before proposing it upstream.
Not intended to be merged upstream from here - just running our own CI for verification.