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

WIP: Adding lettable operators #2667

Merged
merged 69 commits into from
Aug 17, 2017
Merged

WIP: Adding lettable operators #2667

merged 69 commits into from
Aug 17, 2017

Conversation

benlesh
Copy link
Member

@benlesh benlesh commented Jun 15, 2017

This is a WIP, do not merge.

Just planting a flag so people see progress being made.

The overall idea here is that inevitably you'll be able to do this:

import { map, filter } from 'rxjs/operators';

myObservable
  .let(filter(x => x % 2 === 0))
  .let(map(x => x + x))
  .subscribe(x => console.log(x))

I've got a lot of plans here, and there's clearly a lot of work to do. I'd just ask people to refrain from making PRs against this branch until they've chatted with me about it first, I'm still getting this idea to gel within this repository, and how we're going to migrate things toward a more T-Rx project.

@benlesh
Copy link
Member Author

benlesh commented Jun 15, 2017

Also of note: There should be NO BREAKING CHANGES in in this PR. This should be able to ship with the next minor version

@rxjs-bot
Copy link

rxjs-bot commented Jun 15, 2017

Warnings
⚠️

❗ Big PR (1)

Messages
📖

(1) : Pull Request size seems relatively large. If Pull Request contains multiple changes, split each into separate PR will helps faster, easier review.

CJS: 3646.0KB, global: 688.1KB (gzipped: 129.5KB), min: 139.4KB (gzipped: 30.0KB)

Generated by 🚫 dangerJS

note arguments length checking in prototype version
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.003%) to 97.731% when pulling 2cc5d75 on benlesh:lettable-start into 70eaafc on ReactiveX:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.001%) to 97.733% when pulling cd7e7dd on benlesh:lettable-start into 70eaafc on ReactiveX:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.0006%) to 97.735% when pulling cd7e7dd on benlesh:lettable-start into 70eaafc on ReactiveX:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.001%) to 97.733% when pulling 9974fc2 on benlesh:lettable-start into 70eaafc on ReactiveX:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.0003%) to 97.734% when pulling 4ccf794 on benlesh:lettable-start into 70eaafc on ReactiveX:master.

NOTE: The name is now `catchError` because `catch` is an invalid name for a function
@coveralls
Copy link

Coverage Status

Coverage increased (+0.002%) to 97.736% when pulling 408a2af on benlesh:lettable-start into 70eaafc on ReactiveX:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.004%) to 97.738% when pulling 408a2af on benlesh:lettable-start into 70eaafc on ReactiveX:master.

NOTE: Since `do` is an invalid function name, it's called `tap`, which comes from older versions of RxJS
@coveralls
Copy link

Coverage Status

Coverage increased (+0.005%) to 97.739% when pulling 2f12572 on benlesh:lettable-start into 70eaafc on ReactiveX:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.003%) to 97.731% when pulling 2f12572 on benlesh:lettable-start into 70eaafc on ReactiveX:master.

NOTE: I am a little worried about a circular dependency here between ConnectableObservable and the lettable refCount
Copy link
Member

@kwonoj kwonoj left a comment

Choose a reason for hiding this comment

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

I think we can move forward for now as changes are mostly non-breaking and no blocking issues to be changes in this specific PR. As discussed probably we need small chunk of PR for easier change tracking.


export type FactoryOrValue<T> = T | (() => T);

export type MonoTypeOperatorFunction<T> = OperatorFunction<T, T>;
Copy link
Member

Choose a reason for hiding this comment

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

nit: we'll likely to have OperatorFunction<T, R=T> in next if we pick this up into, possible candidate for breaking changes.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.4%) to 97.377% when pulling 762a4f9 on benlesh:lettable-start into d2a32f9 on ReactiveX:master.

@jasonaden
Copy link
Collaborator

It looks like both compose and pipe are left-to-right implementations. I believe most functional implementations of compose are right-to-left (so the value ends up being the return of the first function) and pipe (or a pipe operator) is used for left-to-right. Should this just be a pipe function as well as the current pipe on Observable.prototype? This would be more consistent for those coming from other functional languages/libraries.

@benlesh
Copy link
Member Author

benlesh commented Aug 17, 2017

@jasonaden... you're exactly right... Ramda's compose is right to left. On it's own, it's not quite "pipe"... hmm

@coveralls
Copy link

coveralls commented Aug 17, 2017

Coverage Status

Coverage increased (+0.002%) to 97.736% when pulling 408a2af on benlesh:lettable-start into 70eaafc on ReactiveX:master.

@benlesh benlesh merged commit de9f2c8 into ReactiveX:master Aug 17, 2017
@marcinnajder
Copy link

marcinnajder commented Aug 18, 2017

hi

when I saw the fallowing code myObservable .let(filter(x => x % 2 === 0)) first time, I was wondering, is really type inference in TypeScript so good that the type of variable x will be inferred correctly ?

I did some investigation and it turned out that TypeScript version 2.4 is required. Does it mean that since RxJS 5.5 TS 2.4 will be required ?

marcin

@niieani
Copy link

niieani commented Sep 1, 2017

Hi @benlesh. What you've done here is awesome!

I've started working on a project already using the lettable operators, however it's a bit of a hassle having to manually build rxjs on each computer that I checkout the source. Would you consider releasing a beta with what's currently on master, so that people could start playing with the new lettables?

Also, I take it all operators will eventually become lettables? I was missing some, like startWith, so I had to write my own for the time being, like this:

import {startWith as startWithOnThis} from 'rxjs/dist/cjs/operator/startWith'
export const startWith = (...elements) => (observable) => startWithOnThis.apply(observable, elements)

One more thing, aside from ramda using compose RTL, is there any other reason for renaming to pipe? I personally think compose sounds better. To me, pipe is an operation you specify after the command, not before the actual command, at least that's how I'm used to it in shell (i.e. a | b | c). Also, compose is used LTR in recompose, and I think in lodash it's called flow (that's a bad name, though).

PS
Love the "This is a WIP, do not merge." annotation next to a violet "MERGED" status. 🤣

@benlesh
Copy link
Member Author

benlesh commented Sep 1, 2017

@nileani ... This will all be in 5.5 as soon as we're done porting them over. It shouldn't be too long. Sorry for the inconvenience.

@lock
Copy link

lock bot commented Jun 6, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Jun 6, 2018
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.

9 participants