Skip to content
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

Wrong typings for RxJS and Most.js versions of makeDOMDriver #860

Closed
kosmogradsky opened this issue Nov 27, 2018 · 8 comments · Fixed by #862
Closed

Wrong typings for RxJS and Most.js versions of makeDOMDriver #860

kosmogradsky opened this issue Nov 27, 2018 · 8 comments · Fixed by #862

Comments

@kosmogradsky
Copy link

@cycle/dom v22.0.0 introduced separate makeDOMDriver() functions for RxJS and Most.js, but Typescript throws an error when I try to use them. Probably because the new separate functions describe no arguments in their types:
8174c25#diff-6ec374a820c34eab57b27ca30d8e6ad8R22
8174c25#diff-01aea6cd37e196dd39a5549985abd670R18

Code to reproduce the issue:

import { h1, VNode } from '@cycle/dom';
import { makeDOMDriver, DOMSource } from '@cycle/dom/lib/cjs/rxjs';
import { run } from '@cycle/rxjs-run';
import { of, Observable } from 'rxjs';

import './styles.css';

type mainFunction = (sources: {
  DOM: DOMSource
}) => {
  DOM: Observable<VNode>
};


const main: mainFunction = () => {
  const vdom$ = of(h1('404'));

  return {
    DOM: vdom$
  };
};

const drivers = {
  DOM: makeDOMDriver('#root')
};

run(main, drivers);

Expected behavior:
Code above compiling without any errors.

Actual behavior:
Typescript throws an error Expected 0 arguments, but got 1. when I pass '#root' to mainDOMDriver().

Versions of packages used:

"@cycle/dom": "^22.0.0",
"@cycle/history": "^7.0.0",
"@cycle/rxjs-run": "^10.1.0",
"typescript": "^3.1.6",
"rxjs": "^6.3.3",
@kosmogradsky
Copy link
Author

kosmogradsky commented Nov 27, 2018

This can be worked around using makeDOMDriver() from @cycle/dom and type assertion.

import { DOMSource } from '@cycle/dom/lib/cjs/rxjs';
import { makeDOMDriver, VNode } from '@cycle/dom';
import { Stream } from 'xstream';
import { Driver } from '@cycle/run';

const drivers = {
  DOM: makeDOMDriver('#root') as unknown as Driver<Stream<VNode>, DOMSource>
};

@jvanbruegge
Copy link
Member

Yeah, sorry, this is an incredibly stupid bug. Will fix that tomorrow
One more reason to figure out how to do #802

@kosmogradsky
Copy link
Author

kosmogradsky commented Nov 27, 2018

I've also encountered some problems with DOMSource interface for Most.js and RxJS:

  1. element() method is missing. This method is mentioned in the docs https://cycle.js.org/api/dom.html
  2. events() function parameters are inconsistent between Most.js and RxJS versions when this function returns a stream of Event

Files for reference:
https://github.com/cyclejs/cyclejs/blob/master/dom/src/most.ts
https://github.com/cyclejs/cyclejs/blob/master/dom/src/rxjs.ts

I think it might be better to use common DOMSource<S> interface that accepts a type parameter of either implementation of a reactive stream.

@jvanbruegge
Copy link
Member

jvanbruegge commented Nov 28, 2018

I think it might be better to use common DOMSource<S> interface that accepts a type parameter of either implementation of a reactive stream.

This sadly does not work as we cannot have generics of generics

@IssueHuntBot
Copy link

@issuehuntfest has funded $60.00 to this issue. See it on IssueHunt

jvanbruegge added a commit that referenced this issue Dec 4, 2018
@jvanbruegge jvanbruegge added this to Short term TODO in jvanbruegge's pipeline Dec 4, 2018
@jvanbruegge jvanbruegge moved this from Short term TODO to In progess in jvanbruegge's pipeline Dec 4, 2018
@IssueHuntBot
Copy link

@jvanbruegge has submitted a pull request. See it on IssueHunt

@IssueHuntBot
Copy link

@staltz has rewarded $54.00 to @jvanbruegge. (Total deposit: $60.00, Repository reward(0%): $0.00, Service fee(10%): $6.00) See it on IssueHunt

@staltz staltz closed this as completed Dec 5, 2018
@jvanbruegge
Copy link
Member

This is now released as 22.1.0

@jvanbruegge jvanbruegge moved this from In progess to Done recently in jvanbruegge's pipeline Dec 10, 2018
musicq pushed a commit to musicq/cyclejs that referenced this issue Feb 15, 2020
fuunnx pushed a commit to fuunnx/cyclejs that referenced this issue Feb 24, 2021
fuunnx pushed a commit to fuunnx/cyclejs that referenced this issue Feb 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging a pull request may close this issue.

4 participants