fix(useObjectUrl): revoke the url the effect created - #226
Merged
childrentime merged 1 commit intoSep 15, 2026
Merged
Conversation
The cleanup read `url` out of the render closure, and that closure is the one from the render *before* `setUrl` applied, so on the first run it is `undefined` and nothing is revoked. Every object URL the hook created leaked for the lifetime of the page: on unmount, and on every change of `object`. Keeping the created URL in a local `const` gives the cleanup the value it is meant to release. Clearing the state when `object` goes away also makes the hook return `undefined` again, which is what its type and its docs say it does. Adds the spec this hook did not have. Three of its four cases fail on main. Co-authored-by: Roshan Ramani <rawsun007@users.noreply.github.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
childrentime
approved these changes
Sep 15, 2026
childrentime
left a comment
Owner
There was a problem hiding this comment.
Verified locally: the spec against main's `index.ts` fails 3/4 (revoke on change, revoke on unmount, drop the url when the object goes away); with the fix 4/4 pass and eslint is clean. Holding the created URL in a local `const` means each effect run revokes exactly the URL it created, which also holds up under StrictMode's double invoke.
Non-blocking, for a follow-up rather than this PR: `interface.ts` still types the argument as required (`object: Blob | MediaSource`), while the docs demo passes `File | undefined` and this fix handles `undefined` explicitly. Making it `object?:` (and re-running `gend`) would drop the `as Blob` cast in the spec.
childrentime
added a commit
that referenced
this pull request
Sep 15, 2026
The implementation has always handled a missing object, and since #226 it clears the url when the object goes away, but the type required one. The documented usage, `useObjectUrl(file)` with `file` held in `useState<File>()`, is `File | undefined` and did not typecheck. The argument is now `object?: Blob | MediaSource`, the generated API tables are regenerated, and the spec loses its `as Blob` cast and gains a case for calling it with nothing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
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.
Description
useObjectUrlnever revokes anything. The cleanup readsurlout of the render closure, and that closure belongs to the render beforesetUrlapplied, so on the first runurlisundefinedand theif (url)guard skips the revoke. Every URL the hook creates leaks for the lifetime of the page — on unmount, and on every change ofobject. The docs and the hook's own description say the opposite ("automatically releases the URL viaURL.revokeObjectURL()when the source changes or the component is unmounted").Holding the created URL in a local
constgives the cleanup the value it is meant to release. Clearing the state whenobjectgoes away is the second half: today the hook keeps returning the URL of an object it no longer has, where the return type saysundefined.The hook had no spec, so this adds one. Three of its four cases fail on main: revoke on change, revoke on unmount, and drop the url when the object goes away. Reverting only
index.tsfails exactly those three.The spec mocks
createObjectURL/revokeObjectURLand records both lists, since jsdom implements neither.One thing worth knowing while writing a test for this hook:
renderHook(() => useObjectUrl(new Blob([...])))loops forever, because a fresh object each render re-triggers the effect that sets state. That is inherent to the dependency and not something this PR changes — the docs example holds the file in state, which is the correct usage. The spec uses two module-level blobs.verification:
pnpm test419 pass across 71 suites, eslint andtsc --noEmitclean.Type of Change
Checklist
the docs already describe the behaviour this restores, so there was nothing to change there.
disclosure, and the reason the AI-policy box is unticked: written by Claude Opus 5 running in Claude Code, on my machine and under my direction. i have not read the diff line by line myself yet. i will tick that box once i have.
🤖 Generated with Claude Code