-
-
Notifications
You must be signed in to change notification settings - Fork 33
Fix/issue 64 promise false class #68
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix/issue 64 promise false class #68
Conversation
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you rebase your code, please, as this stuff has already been merged from another PR?
src/sinks/class-sink.test.ts
Outdated
| expect(el.className).not.toContain('class3'); | ||
| }); | ||
|
|
||
| it('should not add class when value is false (promise resolved to false)', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great, can we just give these a little bit of structure, please, so it's easy to see how these are organised, what they are covering and ultimately if there's anything that might be missing, or if new tests need adding, where exactly should those be put?
describe('when a property of the object is a present value', () => {
...
});
describe('when a property of the object is a future value', () => {
describe('when it resolves/emits true', () => {
it('should add a class corresponding to the property name', () => {
describe('when it resolves/emits false () => {
it('should not add a class corresponding to the property name', () => {
src/sinks/class-sink.ts
Outdated
| // 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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any chance we can use the subscribe function, here, instead? That should also flatten promises/observables down
|
adding |
6c5316c to
6ba097f
Compare
| // 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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
was it not feasible to use subscribe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked into using subscribe, but it requires a node parameter as the first argument:
export const subscribe = (node: Node, source: MaybeFuture<T>, next: ...) => { ... }Since we're inside the forEach callback and don't have the node in scope at that point, asap seemed like the right fit. It's also the pattern used in other sinks like content-sink.ts, dataset-sink.ts, and style-sink.ts.
Should I refactor this differently?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, yeah, you're right... good point.
So, we're all good, then, the rest looks great, LGTM!
|
@TudorGR all good and working, great work! |
Summary
Fixes #64 the bug where a Promise resolving to
falsewould incorrectly add a class name to an element.The Problem
When using
class="${{dotted}}"wheredotted = Promise.resolve(false), the class was being added even though the promise resolved tofalse. This happened becauseClassObjectSinkwas checking if the Promise object itself was truthy (it always is) instead of waiting for the resolved value.The Solution
Modified
ClassObjectSinkto:isFuture()Changes
isFuturecheck and conditional logicTesting
All tests pass ✅:
falsedoes NOT add classtrueDOES add classfalsevalue does NOT add class (baseline)Example