[miniflare] Respect fit/gravity/background on the local Images binding transform path#14714
Closed
allocsys wants to merge 19 commits into
Closed
[miniflare] Respect fit/gravity/background on the local Images binding transform path#14714allocsys wants to merge 19 commits into
allocsys wants to merge 19 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 #14695
🦋 Changeset detectedLatest commit: 3d02447 The changes in this PR will be included in the next version bump. This PR includes changesets to release 8 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
workers-devprod
requested review from
a team and
emily-shen
and removed request for
a team
July 16, 2026 05:47
Contributor
|
Codeowners approval required for this PR:
Show detailed file reviewers
|
@cloudflare/autoconfig
@cloudflare/config
create-cloudflare
@cloudflare/deploy-helpers
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-pool-workers
@cloudflare/workers-auth
@cloudflare/workers-editor-shared
@cloudflare/workers-utils
wrangler
commit: |
emily-shen
reviewed
Jul 21, 2026
Contributor
|
Codeowners approval required for this PR:
Show detailed file reviewers
|
…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.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
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.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…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.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…ded, matches production-correct default/scale-down fit behavior after resolveFit/resolveImagesBindingFit split)
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
… 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.
Previous buffering fix (5fd60db) did not resolve the failures - still "Input buffer contains unsupported image format" on all 6 tests. Adding a status/text dump so we can see what's actually coming back instead of guessing again.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Found it via debug logging: the transform response was a 500 with "Error: To use the new ReadableStream() constructor, enable the streams_enable_constructors compatibility flag" - not corrupted image bytes as previously assumed. The Miniflare instance in this test never set compatibilityDate, so it defaulted to a date before that flag is implied. Sibling test images/index.spec.ts already uses compatibilityDate: "2025-04-01" for the same binding - match it here. Reverts the unnecessary request.arrayBuffer() buffering from 5fd60db (request.body streaming was never the problem) and removes the debug logging from cb65af1, now that the actual cause is confirmed.
…ensions (50x25, 2:1) had matching aspect ratios, so fit:cover needed zero cropping and gravity was never actually exercised - gravity:top passed by coincidence, gravity:bottom correctly failed. Use width:100 height:25 (4:1) to force vertical cropping so gravity has an observable effect.
…tom-crop's first row exactly on the white/red seam (original row 50), so Sharp's resampling interpolation blended the two colors ([255,14,14] instead of pure red) - not a logic bug, just sampling too close to the boundary. Use width:200 height:20 (scale=1, pure crop with no resampling) so the kept rows sit solidly inside the white (top) or red (bottom) half.
Contributor
Author
|
Thanks for the look, @emily-shen — the test failures are fixed now. Summary of what was going on and what changed since your review: Root causes found and fixed (all in
Current CI status on
Both look like standard CI infra flakes rather than real regressions — happy to dig further if you'd like, but otherwise this should be ready for another look. |
allocsys
added a commit
to allocsys/workers-sdk
that referenced
this pull request
Jul 22, 2026
…PR closed when source fork was accidentally deleted; content recovered from cloudflare/workers-sdk commit df31b91 via the retained PR ref)
2 tasks
Contributor
Author
|
This PR got closed by accident — the source fork ( I've restored the final reviewed state (commit One thing didn't carry over: the regenerated @emily-shen — apologies again, and thank you for the review so far. Continuing on #14810. |
allocsys
added a commit
to allocsys/workers-sdk
that referenced
this pull request
Jul 22, 2026
…covering cloudflare#14714's stale df31b91 snapshot) while keeping the PR's fit/gravity/background changes
5 tasks
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
3 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.
Summary
Fixes the local Images binding transform path in Miniflare so that
fit,gravity, andbackgroundoptions are respected when processing image transforms locally, matching production Images binding behavior.Fixes #14695
Tests
Added
packages/miniflare/test/plugins/images/transform.spec.tscovering thefit,gravity, andbackgroundtransform options on the local Images binding path, plus a regenerated snapshot for the pre-existingget-platform-proxyImage bindings fixture test that this bug was breaking.Documentation
A changeset is included (
.changeset/fix-images-binding-fit-gravity-background.md).