Skip to content

feat(core): fall back to defaults when Box properties are unset - #1262

Open
posva wants to merge 1 commit into
anomalyco:mainfrom
posva:feat/box-default-fallbacks
Open

feat(core): fall back to defaults when Box properties are unset#1262
posva wants to merge 1 commit into
anomalyco:mainfrom
posva:feat/box-default-fallbacks

Conversation

@posva

@posva posva commented Jul 12, 2026

Copy link
Copy Markdown
Contributor

Hello, while working on Vue TermUI, I faced this "cannot reset to default" problem quite often, so here is a proposal to make it better, starting with the Box renderable.

Why

Frameworks like Vue, React and Solid don't set props once — they update them over time. When a prop is removed from a template or JSX, the framework sets the property to undefined (or null) and expects the renderable to go back to its default value. Right now that doesn't work: defaults are copied into private fields when the renderable is created, so once a value is set there is no way back to "unset".

Two places where this hurts:

  • Correctness: <box :border-color="isActive ? '#f00' : undefined"> should show the default border color when inactive, not the last color that was set.
  • HMR: during hot reload, the framework updates the existing renderables instead of recreating them. If you delete a prop in your code, the old value stays on screen until you restart the app.

The idea (only applied to BoxRenderable for now)

  • Private fields keep the raw value or undefined — the default is never copied in.
  • Public getters return value ?? default, from a module-level BOX_DEFAULTS object. The default is applied when reading, so clearing a property always works.
  • Setters accept undefined to mean "go back to the default".

Box is a good test case because it has an extra problem: the border has no single source of truth. Setting any of borderStyle, borderColor, focusedBorderColor or customBorderChars turns the border on (#186 — frameworks set props one by one, after the constructor ran), and border itself can also be set directly. Before, this worked by mutating _border from an initializeBorder() hook: setting one property could change another behind your back, and removing a prop could never undo it.

Now border is computed: an explicit border value wins; otherwise the border is on if any border-related property is set; otherwise the default (false). One helper, syncBorderSides(), recomputes borderSides and the yoga edges after every border-related change, so a setter can't leave the layout out of sync.

Behavior changes

  • Unsetting any of the migrated props (backgroundColor, border, borderStyle, borderColor, focusedBorderColor, customBorderChars, titleAlignment, bottomTitleAlignment) now brings back the default.
  • An explicit border: false now wins over the other border props (before, they silently turned the border on anyway).
  • Setting focusedBorderColor requests a render even when not focused (it can turn the border on or off, which changes the layout).

Store raw (possibly undefined) values and derive defaults in getters so
that clearing a property restores the original default. Border-related
properties now derive the implied border from one place, replacing the
initializeBorder hack (anomalyco#186). Explicit border: false now wins over
border-implying props.
@kommander

Copy link
Copy Markdown
Collaborator

I think that makes sense and I would support that. I had GPT Sol do a review pass and here are its findings:

  1. High: shared mutable default colors leak across boxes.
    Box.ts:17-22,123-125,177-190 stores module-level RGBA instances and returns them directly. Since RGBA is mutable, changing a.backgroundColor.a also changes every unset box’s background. The same applies to both border colors. Base/main created independent instances.

  2. High: changing borderStyle destroys an unchanged customBorderChars prop.
    Box.ts:164-173 clears both custom-character fields. React and Solid only resend changed props, so this sequence fails:

<box customBorderChars={BorderChars.double} borderStyle={style()} />
// undefined -> "heavy" -> undefined

The custom prop remains present, but the renderable forgets it and ultimately turns the border off. Confirmed end-to-end in both React and Solid.

  1. Medium: Solid spread and style omissions never reach these setters.
    packages/solid/src/renderer/universal.js:191-205 and reconciler.ts:325-331 iterate only new keys:
<box {...props()} /> // { borderStyle: "double" } -> {}
<box style={style()} /> // { backgroundColor: "red" } -> {}

The border remains enabled and the background remains red. Direct signal props work, but the PR does not fulfill its stated behavior for supported Solid APIs.

  1. Medium: shouldFill still cannot restore its default.
    Although included in BOX_DEFAULTS, it remains a plain field at Box.ts:66,88. Transitioning false -> undefined leaves shouldFill === undefined, rather than restoring true. Confirmed through Solid. Thus this is not a complete Box-default fallback implementation.

  2. Medium: exported subclass compatibility is broken.
    The PR removes protected _defaultOptions and makes several protected fields optional at Box.ts:57-71. External BoxRenderable subclasses accessing _defaultOptions stop compiling; subclasses reading _backgroundColor.a can now receive undefined. The repository documents subclassing, though no in-tree subclass currently accesses these fields.

  3. Low: same-object custom-character reassignment no longer updates.
    The identity guard at Box.ts:115-120 prevents reconversion after callers mutate a BorderCharacters object and reassign it. Base/main always rebuilt the native array.

  4. Low: React omission violates the getter type.
    React removes absent props with null at packages/react/src/utils/index.ts:115-120. The custom-character setter stores that verbatim, so customBorderChars returns null despite declaring BorderCharacters | undefined. Confirmed end-to-end.

@posva

posva commented Jul 13, 2026

Copy link
Copy Markdown
Contributor Author
  1. that assumes it's okay to do

    const box = new BoxRenderable(renderer, {})
    box.backgroundColor.a = 0.5

    I agree it's worth fixing. Depending on the properties doing this might be enough

      public get backgroundColor(): RGBA {
        return this._backgroundColor ??= BOX_DEFAULTS.backgroundColor.copy()
      }
  2. does having both props make sense?

  3. did that behavior changed in any way? I don't think so

  4. (seems unrelated) shouldFill maybe shouldnt' be public? it's not exposed as a prop and could be controlled directly via backgroundColor. I think that's beyond my proposal though but interesting!

  5. If extending directly and using protected properties that start with _ was intended, this would require more work indeed, or accept the breaking change as an improvement

  6. wat

  7. yes, I saw that while working on the refactor and prompting (I don't use react myself so I didn't know). We could allow | null on top of undefined, it's technically more correct

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