Skip to content

fix(useObjectUrl): revoke the url the effect created - #226

Merged
childrentime merged 1 commit into
childrentime:mainfrom
rawsun007:fix/use-object-url-revoke
Sep 15, 2026
Merged

childrentime merged 1 commit into
childrentime:mainfrom
rawsun007:fix/use-object-url-revoke

Conversation

@rawsun007

Copy link
Copy Markdown
Contributor

Description

useObjectUrl never revokes anything. The cleanup reads url out of the render closure, and that closure belongs to the render before setUrl applied, so on the first run url is undefined and the if (url) guard skips the revoke. Every URL the hook creates leaks for the lifetime of the page — on unmount, and on every change of object. The docs and the hook's own description say the opposite ("automatically releases the URL via URL.revokeObjectURL() when the source changes or the component is unmounted").

Holding the created URL in a local const gives the cleanup the value it is meant to release. Clearing the state when object goes away is the second half: today the hook keeps returning the URL of an object it no longer has, where the return type says undefined.

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.ts fails exactly those three.

The spec mocks createObjectURL/revokeObjectURL and 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 test 419 pass across 71 suites, eslint and tsc --noEmit clean.

Type of Change

  • Bug fix
  • New hook
  • Enhancement to existing hook
  • Documentation update
  • Other (please describe)

Checklist

  • I have read the Contributing Guide
  • I have read and understood every line of this diff myself, including any AI-written parts (AI usage policy)
  • My code follows the project's coding style
  • I have added tests for my changes
  • All existing tests pass
  • I have updated the documentation

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

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 childrentime left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
childrentime merged commit 9fe2aef into childrentime:main Sep 15, 2026
4 checks passed
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants