MockedDOMSource is not assignable to DOMSource

[ryota-ka]

The release of @cycle/dom v22.2.0 seems to have resulted in introducing a breaking change, though it's a minor version bump.

In 90645d6, DOMSource has been rewritten to a union type, and MockedDOMSource is intentionally excluded from this union.

cyclejs/dom/src/DOMSource.ts

Lines 14 to 16 in c0bb763

	// There is no MockedDOMSource as its functions return any,
	// which would overshadow the other members, making this union pointless
	export type DOMSource = MainDOMSource | DocumentDOMSource | BodyDOMSource;

Before this commit, it was defined as an interface, which allowed us to assign MockedDOMSource to DOMSource.

cyclejs/dom/src/DOMSource.ts

Lines 11 to 23 in 79344a4

	export interface DOMSource {
	select(selector: string): DOMSource;
	elements(): MemoryStream<
	Array<Document> | Array<HTMLBodyElement> | Array<Element> | string
	>;
	element(): MemoryStream<Document | HTMLBodyElement | Element | string>;
	events<K extends keyof HTMLElementEventMap>(
	eventType: K,
	options?: EventsFnOptions,
	bubbles?: boolean
	): Stream<HTMLElementEventMap[K]>;
	events(eventType: string, options?: EventsFnOptions): Stream<Event>;
	}

As a result, MockDOMSource is no longer assignable to DOMSource.

---

Additionally, it seems that we don't have any precise types to which both DOMSource and MockedDOMSource can be assign.

type Sources = { DOM: T }; // what should we expect for T?
type Sinks = { DOM: Stream<VNode> };

function Component(sources: Sources): Sinks {
    // ...
}

Of course we can define something like type T = DOMSource | MockedDOMSource, but this definition tsc will claim as following (as of typescript v3.2.2).

path/to/Component.ts:156:9 - error TS2347: Untyped function calls may not accept type arguments.

156         DOM.select('.incr')
            ~~~~~~~~~~~~~~~~~~~
157             .events('click')
    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
158             .mapTo(1),
    ~~~~~~~~~~~~~~~~~~~~~~

To me, it appears that it is not a very good idea to define DOMSource as a union type, as TypeScript's inference over structual subtypes are not strong enough. (Or can it be possibly fixed with better typings?)
If possible, it would be great that DOMSource is defined as an interface again, with enabling TypeScript's strict mode.

Thanks!

[staltz]

Thanks for reporting this, @ryota-ka, I was fearing that this use case could appear as a breaking change, and it indeed did. The move to union type was a clever way of allowing us to "delete" types through TypeScript's "type ternary condition".

I understand your need for an interface.

---

@jvanbruegge Perhaps we could have some internal type used _DOMSource which is a union type, and then DOMSource which is an externally provided interface (or object type alias)? Not sure if that works, but just trying to look for a solution.

[akaan]

Is there any workaround for this or should I downgrade to a previous version to use mockDOMSource in a TypeScript project ?

[ryota-ka]

@staltz @jvanbruegge While investigating this (after a year!), I've noticed this is a problem caused by the variance of Streams and MemoryStreams.
(see also: staltz/xstream#286)

I found that @jvanbruegge has supported TypeScript's strict mode in 90645d6, but until the time we have {co,conta}variant subtypes of Streams, how about disabling strictFunctionTypes option?

My commit fixing this is here: ryota-ka@e8532c9
If it looks okay, I can submit a pull request.

[akaan]

Waiting for the fix, I've been doing the following :

export function intent(DOM: DOMSource | MockedDOMSource): Actions {
  // https://github.com/cyclejs/cyclejs/issues/869
  if (DOM instanceof MockedDOMSource) {
    return {
      addOne$: DOM.select(".add")
        .events("click")
        .mapTo(null),
      removeOne$: DOM.select(".remove")
        .events("click")
        .mapTo(null)
    };
  } else {
    return {
      addOne$: DOM.select(".add")
        .events("click")
        .mapTo(null),
      removeOne$: DOM.select(".remove")
        .events("click")
        .mapTo(null)
    };
  }
}

This is quite ugly, is there a better way ?

[jvanbruegge]

Sadly not at the moment. This won't be an issue in the new version, but that will still take a bit of time. Another workaround (what I've been doing) is in the tests type MockedDOMSource as any and pass that in. Then you don't need that ugly if/else duplication