From c1f81bf727abadc2764baaf16bc58cf1ba75ddf7 Mon Sep 17 00:00:00 2001 From: Tudor Gradinaru Date: Wed, 29 Oct 2025 17:50:17 +0200 Subject: [PATCH 1/2] Fix #64: Handle promises resolving to false in ClassObjectSink --- src/sinks/class-sink.test.ts | 46 ++++++++++++++++++++++++++++++++++++ src/sinks/class-sink.ts | 11 ++++++++- 2 files changed, 56 insertions(+), 1 deletion(-) diff --git a/src/sinks/class-sink.test.ts b/src/sinks/class-sink.test.ts index 51bbe1f..af142f5 100644 --- a/src/sinks/class-sink.test.ts +++ b/src/sinks/class-sink.test.ts @@ -61,6 +61,52 @@ describe('Class Sink', () => { expect(el.className).not.toContain('class3'); }); + it('should not add class when value is false (promise resolved to false)', () => { + const el = MockElement(); + const sink = ClassObjectSink(el); + + // Simulating a promise that resolved to false + sink({ + dotted: false, + }); + + expect(el.className).not.toContain('dotted'); + expect(el.className).toEqual(''); + }); + + it('should not add class when promise resolves to false', async () => { + const el = MockElement(); + const sink = ClassObjectSink(el); + + // Pass a promise that resolves to false + const dottedPromise = Promise.resolve(false); + sink({ + dotted: dottedPromise, + }); + + // Wait for the promise to resolve + await dottedPromise; + + expect(el.className).not.toContain('dotted'); + expect(el.className).toEqual(''); + }); + + it('should add class when promise resolves to true', async () => { + const el = MockElement(); + const sink = ClassObjectSink(el); + + // Pass a promise that resolves to true + const activePromise = Promise.resolve(true); + sink({ + active: activePromise, + }); + + // Wait for the promise to resolve + await activePromise; + + expect(el.className).toContain('active'); + }); + }); }); diff --git a/src/sinks/class-sink.ts b/src/sinks/class-sink.ts index ac699a4..9690117 100644 --- a/src/sinks/class-sink.ts +++ b/src/sinks/class-sink.ts @@ -4,6 +4,7 @@ import type { Sink, ExplicitSink } from "../types/sink"; import { SINK_TAG } from "../constants"; import { asap } from "../lib/drain"; +import { isFuture } from "../types/futures"; export const TOGGLE_CLASS_SINK_TAG = 'ToggleClass'; export const CLASS_SINK_TAG = 'class'; // Keeping it same as 'class" attribute for now. Don't change yet... @@ -35,7 +36,15 @@ export const ClassObjectSink: Sink = (node: Element) => { // FIXME: is it safe to assume it's an object, at this point? : (<(ClassName | ClassRecord)[]>[]).concat(name).forEach(obj => Object.entries(obj) // TODO: support 3-state with toggle - .forEach(([k, v]) => asap(v ? add : remove, k)) + .forEach(([k, v]) => { + // If v is a future (Promise or Observable), defer the decision + // until it resolves, otherwise check immediately + if (isFuture(v)) { + asap((resolvedValue: any) => resolvedValue ? add(k) : remove(k), v); + } else { + asap(v ? add : remove, k); + } + }) ) }; }; From 6ba097ff43a654a1ff6b10acda18ad10397fd803 Mon Sep 17 00:00:00 2001 From: Tudor Gradinaru Date: Fri, 31 Oct 2025 16:13:24 +0200 Subject: [PATCH 2/2] Address PR review feedback: restructure tests and simplify asap usage --- src/sinks/class-sink.test.ts | 125 +++++++++++++++++++---------------- src/sinks/class-sink.ts | 12 ++-- 2 files changed, 72 insertions(+), 65 deletions(-) diff --git a/src/sinks/class-sink.test.ts b/src/sinks/class-sink.test.ts index af142f5..69d9f6e 100644 --- a/src/sinks/class-sink.test.ts +++ b/src/sinks/class-sink.test.ts @@ -30,81 +30,92 @@ describe('Class Sink', () => { describe('Given a class object', () => { - it('sets classes for truthy attributes on sink', () => { - const el = MockElement(); - const sink = ClassObjectSink(el); + describe('when a property of the object is a present value', () => { + + it('sets classes for truthy attributes on sink', () => { + const el = MockElement(); + const sink = ClassObjectSink(el); + + sink({ + class1: true, + class2: 1, + class3: 'yes!', + }); + expect(el.className).toContain('class1'); + expect(el.className).toContain('class2'); + expect(el.className).toContain('class3'); + }); - sink({ - class1: true, - class2: 1, - class3: 'yes!', + it('clears classes for falsy attributes on sink', () => { + const el = MockElement({ className: 'class1 class2 class3' }); + const sink = ClassObjectSink(el); + expect(el.className).toContain('class1'); + expect(el.className).toContain('class2'); + expect(el.className).toContain('class3'); + + sink({ + class1: false, + class2: 0, + class3: '', + }); + expect(el.className).not.toContain('class1'); + expect(el.className).not.toContain('class2'); + expect(el.className).not.toContain('class3'); }); - expect(el.className).toContain('class1'); - expect(el.className).toContain('class2'); - expect(el.className).toContain('class3'); - }); - it('clears classes for falsy attributes on sink', () => { - const el = MockElement({ className: 'class1 class2 class3' }); - const sink = ClassObjectSink(el); - expect(el.className).toContain('class1'); - expect(el.className).toContain('class2'); - expect(el.className).toContain('class3'); + it('should not add class when value is false', () => { + const el = MockElement(); + const sink = ClassObjectSink(el); - sink({ - class1: false, - class2: 0, - class3: '', + sink({ + dotted: false, + }); + + expect(el.className).not.toContain('dotted'); + expect(el.className).toEqual(''); }); - expect(el.className).not.toContain('class1'); - expect(el.className).not.toContain('class2'); - expect(el.className).not.toContain('class3'); + }); - it('should not add class when value is false (promise resolved to false)', () => { - const el = MockElement(); - const sink = ClassObjectSink(el); + describe('when a property of the object is a future value', () => { - // Simulating a promise that resolved to false - sink({ - dotted: false, - }); + describe('when it resolves/emits false', () => { - expect(el.className).not.toContain('dotted'); - expect(el.className).toEqual(''); - }); + it('should not add a class corresponding to the property name', async () => { + const el = MockElement(); + const sink = ClassObjectSink(el); - it('should not add class when promise resolves to false', async () => { - const el = MockElement(); - const sink = ClassObjectSink(el); + const dottedPromise = Promise.resolve(false); + sink({ + dotted: dottedPromise, + }); + + await dottedPromise; + + expect(el.className).not.toContain('dotted'); + expect(el.className).toEqual(''); + }); - // Pass a promise that resolves to false - const dottedPromise = Promise.resolve(false); - sink({ - dotted: dottedPromise, }); - // Wait for the promise to resolve - await dottedPromise; + describe('when it resolves/emits true', () => { - expect(el.className).not.toContain('dotted'); - expect(el.className).toEqual(''); - }); + it('should add a class corresponding to the property name', async () => { + const el = MockElement(); + const sink = ClassObjectSink(el); - it('should add class when promise resolves to true', async () => { - const el = MockElement(); - const sink = ClassObjectSink(el); + const activePromise = Promise.resolve(true); + sink({ + active: activePromise, + }); - // Pass a promise that resolves to true - const activePromise = Promise.resolve(true); - sink({ - active: activePromise, - }); + await activePromise; - // Wait for the promise to resolve - await activePromise; + expect(el.className).toContain('active'); + }); + + }); - expect(el.className).toContain('active'); }); }); diff --git a/src/sinks/class-sink.ts b/src/sinks/class-sink.ts index 9690117..a0688df 100644 --- a/src/sinks/class-sink.ts +++ b/src/sinks/class-sink.ts @@ -4,7 +4,6 @@ import type { Sink, ExplicitSink } from "../types/sink"; import { SINK_TAG } from "../constants"; import { asap } from "../lib/drain"; -import { isFuture } from "../types/futures"; export const TOGGLE_CLASS_SINK_TAG = 'ToggleClass'; export const CLASS_SINK_TAG = 'class'; // Keeping it same as 'class" attribute for now. Don't change yet... @@ -37,13 +36,10 @@ export const ClassObjectSink: Sink = (node: Element) => { : (<(ClassName | ClassRecord)[]>[]).concat(name).forEach(obj => Object.entries(obj) // TODO: support 3-state with toggle .forEach(([k, v]) => { - // If v is a future (Promise or Observable), defer the decision - // until it resolves, otherwise check immediately - if (isFuture(v)) { - asap((resolvedValue: any) => resolvedValue ? add(k) : remove(k), v); - } else { - asap(v ? add : remove, k); - } + // Use asap to handle both present and future values + // For futures (Promise/Observable), it will wait for resolution + // For present values, it will execute immediately + asap((resolvedValue: any) => resolvedValue ? add(k) : remove(k), v); }) ) };