Skip to content

feat(useStorage): add an onError hook for failed writes - #767

Open
sridhar-3009 wants to merge 1 commit into
vuetifyjs:masterfrom
sridhar-3009:feat/storage-on-error-hook-756
Open

feat(useStorage): add an onError hook for failed writes#767
sridhar-3009 wants to merge 1 commit into
vuetifyjs:masterfrom
sridhar-3009:feat/storage-on-error-hook-756

Conversation

@sridhar-3009

Copy link
Copy Markdown
Contributor

Closes #756.

createStorage's deep-watcher write path catches adapter errors — a full localStorage quota, a SecurityError in a restricted context, an adapter-level failure — and routes them only to the internal logger (writeStored catch → logger.error('[v0:storage] Failed to write key …')) without rethrowing. Writes are fire-and-forget deep-watcher side effects, so consumers have no programmatic way to learn a write failed: storage.set cannot reject, and no error state is exposed. The user keeps editing, believing their data is persisted, while it silently is not.

Proposal implemented

An opt-in onError hook on createStorage / createStoragePlugin, per the issue's own proposal:

createStoragePlugin({
  onError: (error, key) => { /* surface to app state */ },
})

Called with the underlying error and the prefixed storage key whenever writeStored throws. The existing internal log call is untouched and still fires alongside it — this is purely additive, not a replacement.

I went with the callback approach over an error ref on the storage context, per the issue's own reasoning: a callback composes better with per-consumer handling (e.g. routing straight into a toast/notification system) and matches the adapter-injection style already used elsewhere in the package (useLogger, useLocale).

Tests

  • onError is called with the error and prefixed key when a write throws.
  • onError is not called when a write succeeds.
  • Existing 'should log error when writeStored fails' test still passes unchanged, confirming the internal log path is untouched.

67 existing + new tests for this composable pass, plus the full 4787-test non-browser suite.

@johnleider johnleider left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for the batch of PRs this weekend — #766 merged as-is, and your #764 surfaced a naming-precedence inconsistency we're fixing in a follow-up. This one is a genuinely useful addition and the implementation is clean (purely additive, unknown over any, the option flows through the plugin with no extra wiring, good changeset copy). Three things before it can land:

  1. Base branch. This adds public API (feat + minor changeset), which targets dev in our branch model — master is the patch train and merging there would push a minor out with the next patch release. Please rebase onto dev.

  2. Coverage vs. claim. The hook only wraps writeStored. The read path is arguably the more common real failure and stays silent: the readStored catch (corrupt/unparseable stored JSON) still only hits the logger, adapter?.getItem sits outside that try so a SecurityError on read throws straight to the caller, and remove()/clear()/the storage-event removeItem have no handling at all. Either extend onError to the read/remove paths, or narrow the option's doc comment and changeset to "write errors" so the API doesn't overpromise — happy with whichever you prefer, extending gets my vote.

  3. Docs. apps/docs/src/pages/composables/plugins/use-storage.md has sections for prefix and ttl; a new public option needs one too, or it ships invisible.

One non-blocking note: elsewhere in v0 (useImage, Avatar/Image components) onError is a handler the composable returns for you to bind, whereas here it's a callback you supply — same name, inverted direction. If you have a better name in you (onWriteError?), now is the cheap time; otherwise this sets the precedent and that's a defensible call too.

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.

feat(useStorage): onError hook so consumers can surface failed writes

2 participants