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

Make .flatScan work seedless #737

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

semmel
Copy link
Member

@semmel semmel commented Jun 10, 2019

If just the reducer function is given, the first value of the source stream will be passed as initial event of the resulting Property and used as the left argument when the reducer is first invoked on the second source event.

I've found this useful to save meaningless lines of code. It should be easy understand because JS users are familiar with Array.reduce where the seed value is also optional. It should also be 100% non-breaking.

Bacon.fromArray([10, 20]).flatScan((a, b) => a + b).log();
// --> 10, 30

If you are in favour of this change, perhaps .scan should be extended as well.

Remarks:

  • I've found it very difficult to please the TypeScript's type checking. Because the first parameter to .flatScan is mow "multi-morphic" (i.e. a value or a function). Which IMO is naturally for dynamic JavaScript however TypeScript makes it really hard to enable this at all?
  • I've not updated the documentation yet.

@semmel
Copy link
Member Author

semmel commented Jun 19, 2019

Hi @raimohanska,
any comment on this PR?

@raimohanska
Copy link
Contributor

I like the idea but hate how it makes the flatScan signature look. This is not easy to do in Typescript. If only the seed parameter was the last one, it would be feasible. Not sure how to proceed.

@semmel
Copy link
Member Author

semmel commented Jun 24, 2019

I agree that the TS signature for this is very troubling! (I spend more time on figuring it out then on the implementation and the tests btw.)

It's just that as a JavaScript programmer I see the .*scan functions as close relatives to Array.prototype.reduce (just with arguments flipped). I learned where that the seed is optional which makes reduce more versatile, so the programmer has not to master different methods.

Maybe now I am exaggerating, but in the end you'll have to decide if Bacon.js should be a TypeScript library which can also be used in JavaScript, or a JavaScript library.

While I certainly appreciate type support in my JS code (JSDoc only goes so far), I find TypeScript too opinionated (esp. strict mode) and not as easy approachable for beginners. Maybe in this case the type system gets in the way of getting things done cleanly and quickly 😞

@raimohanska
Copy link
Contributor

Yes, in this case the Typescript type system gets in the way, because it doesn't allow the kind of method overloading we'd like to have. Damn it :)

@raimohanska
Copy link
Contributor

raimohanska commented Jun 27, 2019

Took a closer look at this.

Just like for Array.reduce, or RxJs scan the seedless version is possible only when input and output types are equal. So type signatures require a bit of honing. This is doable!

Another thing: checking for typeof the first argument will fail for streams containing functions as values. This could probably be replaced by argument count check.

Third: For consistency, the same improvement should be applied to scan, fold as well.

@raimohanska
Copy link
Contributor

I pushed some improvements to to https://github.com/baconjs/bacon.js/commits/seedless-flatscan

Used overloading to keep the signatures nice, replaced typeof checking with argument count checking. What do you think?

For consistency, scan and fold should be updated as well, though.

@raimohanska
Copy link
Contributor

So now the method has two, decent-looking signatures:

  flatScan<V2>(seed: V2, f: Function2<V2, V, Observable<V2>>): Property<V2>

  flatScan<V2>(f: Function2<V2, V, Observable<V2>>): Property<V2>

@raimohanska
Copy link
Contributor

Oh, that's not nice.

@raimohanska
Copy link
Contributor

This is better

  flatScan<V2>(seed: V2, f: Function2<V2, V, Observable<V2>>): Property<V2>

  flatScan(f: Function2<V, V, Observable<V>>): Property<V>

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

Successfully merging this pull request may close these issues.

None yet

2 participants