Skip to content
This repository has been archived by the owner on Dec 18, 2019. It is now read-only.

Support Cycle Unified #18

Merged
merged 4 commits into from
Apr 11, 2017
Merged

Support Cycle Unified #18

merged 4 commits into from
Apr 11, 2017

Conversation

goodmind
Copy link
Contributor

@goodmind goodmind commented Apr 4, 2017

No description provided.

src/index.ts Outdated
@@ -14,23 +15,25 @@ export type Lens<T, R> = {
}
export type Scope<T, R> = string | number | Lens<T, R>;

export function pick(selector: Selector | string) {
export function pick<T>(selector: Selector | string) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @goodmind, thanks for the PR.
Could you revert the changes in this part? I know it makes the typings generic for any stream library, but I don't want to make these typings less useful for xstream users. I think the approach should be like in Cycle DOM or Cycle HTTP, where RxJS users import rxjs-typings.d.ts. If that's not possible, then RxJS and most.js users should just use any casting.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can use here generic defaults microsoft/TypeScript#13487

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will be even better if TypeScript supports Higher Kinded Types

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can when TypeScript 2.3 is out. https://github.com/Microsoft/TypeScript/milestone/38
We can either revert these changes, and later update to 2.3, or wait until 2.3 is out.

src/index.ts Outdated
export function mix(aggregator: Aggregator) {
return function mixOperator(streamArray$: Stream<Array<Stream<any>>>): Stream<any> {
return streamArray$
export function mix<T>(aggregator: Aggregator) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

src/index.ts Outdated
@@ -115,7 +118,7 @@ export class StateSource<T> {
public select<R>(scope: Scope<T, R>): StateSource<R> {
const get = makeGetter(scope);
return new StateSource<R>(
this.state$.map(get).filter(s => typeof s !== 'undefined'),
xs.fromObservable<T>(this.state$).map(get).filter(s => typeof s !== 'undefined'),
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can avoid this one since RxJS/xstream/most.js all have map and filter. It should save us something in performance.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to #26
This stream passed to constructor where compose called on it

src/index.ts Outdated
@@ -134,7 +137,8 @@ export default function onionify<So, Si>(
.drop(1);
sources[name] = new StateSource<any>(state$, name) as any;
const sinks = main(sources as So);
reducerMimic$.imitate(sinks[name]);
const imitation$ = sinks[name] && xs.fromObservable(sinks[name]);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why this line is needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because without it tests failing, there is two tests with undefined sink and when we do xs.fromObservable(undefined) it fails

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay. Can we use a traditional if ( ) { block here?

@goodmind
Copy link
Contributor Author

goodmind commented Apr 8, 2017

@staltz any comments?

@staltz
Copy link
Owner

staltz commented Apr 11, 2017

Looks good, thanks @goodmind

@staltz staltz merged commit e8e6c5d into staltz:master Apr 11, 2017
@staltz
Copy link
Owner

staltz commented Apr 11, 2017

New version v3.2.0

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants