ReviewOS

stacks/stx

useColorMode: cannot apply both a class and an attribute, and cannot persist a 'system' preference

#1788
Closed glennmichael123 opened this 10 days ago ·
10 days ago

Summary

useColorMode is the right abstraction for theme handling, but two details in its current implementation make it unusable for an app that already has a theme convention, forcing a fallback to imperative document.documentElement calls in a <script client> block.

Both are in useColorMode in the signals runtime (generateSignalsRuntimeDev, dist/signals.js).

1. applyDOM sets a class or an attribute, never both

function applyDOM(mode) {
  var el = document.documentElement;
  if (disableTransitions) el.style.setProperty('transition', 'none', 'important');
  if (attribute) { el.setAttribute(attribute, mode); }
  else { if (mode === 'dark') el.classList.add(darkClass); else el.classList.remove(darkClass); }
  ...
}

Passing attribute silently disables darkClass. An app needing both — a html.dark class because every dark: utility and CSS-variable override keys off it, plus data-theme for non-utility CSS and for reading current state — cannot express that.

They are not alternatives in practice: the class drives the utility framework, the attribute drives everything else.

Suggested fix: apply both when both are configured. darkClass already defaults to 'dark' and attribute defaults to null, so applying both whenever attribute is non-null is backward compatible for anyone relying on the current class-only default.

if (attribute) el.setAttribute(attribute, mode);
if (darkClass) el.classList.toggle(darkClass, mode === 'dark');

2. The persisted vocabulary is hard-coded to light | dark | auto

function readPersisted() {
  try { var v = localStorage.getItem(storageKey);
    if (v === 'light' || v === 'dark' || v === 'auto') return v; }
  catch (e) {}
  return null;
}

storageKey is configurable but the accepted values are not. An app that already persists 'system' (a common spelling for the same concept) hits a silent data-loss path:

  1. readPersisted() sees 'system', rejects it, returns null.
  2. update(persisted || initialMode) falls back to 'auto'.
  3. persist('auto') overwrites the stored 'system'.

Any other code reading that key — typically a pre-paint inline script in the layout, which cannot be a hydrated directive without a flash — then sees a value it does not recognise and renders the wrong theme. The failure is silent and only shows up on reload.

Suggested fix: either accept 'system' as an alias for 'auto', or add a modes/autoValue option so the persisted vocabulary can match an existing convention. At minimum, do not overwrite a stored value that was not recognised.

Why this matters

These push apps back to hand-written classList.toggle + setAttribute + localStorage for theming — the exact imperative DOM code the signals/directives model exists to remove. With both addressed, useColorMode fully replaces that block.

Environment

  • @stacksjs/stx as shipped in node_modules/@stacksjs/stx/dist/signals.js
  • Bun 1.4.0-canary.1

Sign in to comment on this issue.