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

feat(operate): Adds a .operate() and alias .o() operator #2034

Closed
wants to merge 1 commit into from

Conversation

jayphelps
Copy link
Member

@jayphelps jayphelps commented Oct 14, 2016

Allows you to apply an operator to a given stream, without it needing to be on the Observable prototype. This is mostly useful for library authors where adding operators to Observable.prototype can cause headaches to their users.

It takes the provided operator function and any arbitrary arguments then internally just calls operator.apply(this, args).

This is solves a similar problem as the TC39 proposed bind operator syntax e.g. stream::map(i => i + 1) but that's stage-0 and not supported by TypeScript. Note that this current implementation lacks type safety/suggestions, just like it would if you used map.call(stream, i => i + 1).

There is also an alias of just the letter o, as in .o(map, i => i + 1) so your code can remain fairly terse.

import { of } from 'rxjs/observable/of';
import { filter } from 'rxjs/operator/filter';
import { map } from 'rxjs/operator/map';

of(1, 2, 3)
  .o(filter, i => i > 1)
  .o(map, i => i * 10)
  .subscribe(x => console.log(x));
// 20..30

Without this, you'd need to something like this, which can get rather hairy quick:

let result = of(1, 2, 3);
result = filter.call(result, i => i > 1);
result = map.call(result, i => i * 10);
result.subscribe(x => console.log(x));

The name operate and having an short alias like o hasn't had a ton of thought into it, just what @Blesh and I came up with in 5 minutes. While I generally hate aliases, the idea was that searching and discovering o in our documentation is basically fruitless, but having a long name is annoying when using these often in a library. We considered ._(operator) .$(operator) .invoke(operator)

Cc/ @IgorMinar ng2 may like this and it adds nearly trivial overhead since it only has the extra function invocation on stream setup, not next()'s so should be out of the hot path for nearly all cases

@coveralls
Copy link

Coverage Status

Coverage increased (+0.002%) to 97.041% when pulling feb379d on jayphelps:operate into 1bee98a on ReactiveX:master.

@benlesh
Copy link
Member

benlesh commented Oct 14, 2016

How does TypeScript handle this when it comes to operators like window or groupBy? Are types inferred properly?

@benlesh
Copy link
Member

benlesh commented Oct 14, 2016

It seems like the first argument should be Function<Observable<R>> or (...args: any[]) => Observable<R> and it should return Observable<R>. Then we'll likely need a few signature overloads for it. @david-driscoll? Thoughts?

@jayphelps
Copy link
Member Author

jayphelps commented Oct 14, 2016

@Blesh not entirely sure of your first question regarding groupBy, but generally best I can tell when using things like ...args Array<any> you lose type info. so certainly using this is not type safe. Nothing we can really do about it AFAIK. It's not any less safe than map.call(stream, i => i + 1) :trollface:

Edit: unless we add every single type signature for all operators, which at first seems a little overboard but perhaps...

@benlesh
Copy link
Member

benlesh commented Oct 14, 2016

@jayphelps, you should be able to use generics for this. operate<R,A,B,C>((a:A, b:B, c:C) => Observable<R>, a:A, b:B, c:C): Observable<R> obviously for many signatures

@jayphelps
Copy link
Member Author

@Blesh I'm still not following. Can you show me what you mean using a more complete example maybe? Add your suggested generics to:

stream.operate(map, i => i + 1)

@benlesh
Copy link
Member

benlesh commented Oct 14, 2016

I just mean for signature overrides. I'll add more when I'm not on my phone.

@IgorMinar
Copy link
Contributor

IgorMinar commented Oct 14, 2016

This is quite intriguing but has one flaw - the types won't flow properly so you will not get operator-specific checking.

I don't know enough about how the operators are internally wired together (I remember something about lift operator being used), but wouldn't it be possible to do something like this:

of(1, 2, 3)
  .o(filter(i => i > 1))
  .o(map(i => i * 10))
  .subscribe(x => console.log(x));

maybe even:

of(1, 2, 3)
  .o(
    filter(i => i > 1),
    map(i => i * 10)
   )
  .subscribe(x => console.log(x));

// cc: @chuckjaz - for other typescript ideas on how to get the types to flow properly.

@jayphelps
Copy link
Member Author

jayphelps commented Oct 14, 2016

@Igorbek yep, the lack of type safety is just like map.call(stream, ...args). e.g. from ng2

This sort of thing:

of(1, 2, 3)
  .o(filter(i => i > 1))
  .o(map(i => i * 10))
  .subscribe(x => console.log(x));

or similar wouldn't work (currently) because operators require this context to call this.lift() when you invoke them. We could create lazy wrappers for all of them...but dang that's a bunch of duplicate type signatures and code. At that point, perhaps it just be easier to add the signatures for all our operators to the .operate() helper?

// .map() signature
o<T>(operator: typeof map, project: (value: T, index: number) => R, thisArg?: any): Observable<T>;
// .filter() signature
o<T>(operator: typeof filter, predicate: (value: T, index: number) => boolean, thisArg?: any): Observable<T>;
// etc
o<T>(operator: Function, ...args: Array<any>): Observable<T> {
  return operator.apply(this, args);
}

It's possible something like your suggestion is more ergonomic though. Not immediately clear for me.

of(1, 2, 3)
  .o(
    filter(i => i > 1),
    map(i => i * 10)
   )
  .subscribe(x => console.log(x));

Either way I hate having to maintain duplicate signatures like this, especially in another file. I have a hunch these will get out of sync--but maybe I'm being paranoid. Personally I'm leaning towards lack of type safety since it's no worse than the existing operator.call(stream) solution but cleaning for chaining. There may be some TS voodoo I'm not aware of to let us reuse the signatures similar to how we can use typeof map for the first param.

@IgorMinar
Copy link
Contributor

I see. This would work as well. If I have custom operators, I should be
able to reopen the type and add an overload. Interesting...

On Thu, Oct 13, 2016 at 9:25 PM Jay Phelps notifications@github.com wrote:

@Igorbek https://github.com/Igorbek yep, the lack of type safety is
just like map.call(stream, ...args).

of(1, 2, 3)
.o(filter(i => i > 1))
.o(map(i => i * 10))
.subscribe(x => console.log(x));

or similar wouldn't work out-of-box because operators require this
context to call this.lift() when you invoke them. We could create lazy
wrappers for all of them...but dang that's a bunch of duplicate type
signatures and code. At that point, perhaps it just be easier to add the
signatures for all our operators?

// .map() signatureo(operator: typeof map, project: (value: T, index: number) => R, thisArg?: any): Observable;// .filter() signatureo(operator: typeof filter, predicate: (value: T, index: number) => boolean, thisArg?: any): Observable;// etco(operator: Function, ...args: Array): Observable {
return operator.apply(this, args);
}


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#2034 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AANM6LCp4ouZlXnZwZtyuk5lsWPt_vOqks5qzwQ3gaJpZM4KWhzT
.

@staltz
Copy link
Member

staltz commented Oct 14, 2016

Why not just stay with let? This looks weird to me and adds yet another way of doing the same thing.

@jayphelps
Copy link
Member Author

Because let is rather verbose to use in this way IMO and the number one reason is that it is not included on the prototype by default--so you can't add it or you're defeating the point. We could consider making it added always by default though.

@david-driscoll
Copy link
Member

Worst case we should be able to augment the operator on Observable<T> inside of each function module. Which would basically be what the signature interfaces were. Except we don't have to worry about augmenting the prototype, overloading the method be sufficient.

@david-driscoll
Copy link
Member

david-driscoll commented Oct 14, 2016

I remember mentioning something on slack, but it's worth raising perhaps.

If we updated the operators to not use this and the observable was instead the first argument. Keep in mind this can be something we either do or undo using a pre-build step. We produce one version, that's used for prototype augmentation, and another version that is exported for the general methods.

The only difference between (as of TypeScript 2.0) is the argument name and the call to this. This way we avoid the perf cost associated wraping each operator in a method users this as the first argument.

export function map<T, R>(this: Observable<T>, project: (value: T, index: number) => R, thisArg?: any): Observable<R> {
  return this.lift(new MapOperator(project, thisArg));
}

// vs
export function map<T, R>(context: Observable<T>, project: (value: T, index: number) => R, thisArg?: any): Observable<R> {
  return context.lift(new MapOperator(project, thisArg));
}

With this we would see something like...

import { of } from 'rxjs/observable/of';
import { filter } from 'rxjs/operator/filter';
import { map } from 'rxjs/operator/map';

map(
  filter(
    of(1, 2, 3),
    i => i > 1
  ),
  i => i * 10
)
  .subscribe(x => console.log(x));

Now that is clearly a little unwieldy, much like the call syntax.

Then if we have this new o method (or perhaps extend let to also apply arguments), we would get:

of(1, 2, 3)
  .let(filter, i => i > 1) // calls func(this, ...) with the given observable.
  .let(map, i => i * 10)
  .subscribe(x => console.log(x));

Also to @IgorMinar's point we could also introduce a new method on each operator itself (eg o or operate. We would get something like... (which wouldn't require changing let at all).

of(1, 2, 3)
  .let(filter.o(i => i > 1))
  .let(map.o(i => i * 10))
  .subscribe(x => console.log(x));

The o method would have an interface something like map.o<T, R>(projector: (value: T) => R): (observable: Observable<T>) => Observable<R>.

On the TypeScript side there would have to be some explicit typing, as there would be no context to find T since map.o would be called before it's handed to the Observable.

This also opens the door for something like lodash's flow

var a = _.flow([
  filter.o(i => i > 1),
  map.o(i => i * 10)
]);

// or

var a = _.flow([
  _.partial(filter, _, i => i > 1),
  _.partial(map, _, i => i * 10)
]);

a(of(1, 2, 3))
  .subscribe(x => console.log(x));

@benlesh
Copy link
Member

benlesh commented Oct 14, 2016

source
  .let(s => filter.call(s, x => x > 1))
  .let(s => map.call(s, x => x + 10))

vs

source
  .op(filter, x => x > 1)
  .op(map, x => x + 10)

otherwise someone could do something like

const letWrap = (operatorFn) => (...args) => (source) => operatorFn.apply(source, args);

const _filter = letWrap(filter);
const _map = letWrap(map);

source
  .let(_filter(x => x > 1))
  .let(_map(x => x + 10))

We could even provide the letWrap function as part of the library, since it's so lightweight. The issue then is that let doesn't necessarily exist on Observable, it would need to be added to the prototype before use.

To be clear though, I feel like the solution proposed in this PR is more ergonomic and efficient.

@benlesh
Copy link
Member

benlesh commented Oct 14, 2016

Another possibility:

source.with(filter, map)
  .filter(x => x > 1)
  .map(x => x + 10)

EDIT: Actually, can't always infer names from the function in all runtimes, so it would need to be a hash or something:

source.with({ filter, map })
  .filter(x => x > 1)
  .map(x => x + 10)

@benlesh
Copy link
Member

benlesh commented Oct 14, 2016

Here is an alternative implementation: #2036

It's based off of my previous comment.

@chuckjaz
Copy link
Contributor

chuckjaz commented Oct 14, 2016

I tried to get the types flowing for each of these proposals. I got the closest with @Blesh's proposal but it is still not great.

The original proposal can be summarized into:

  interface O<T> {
    o(o: (this: O<T>, p: (n: T) => boolean) => O<T>, p: (n: T) => boolean): O<T>;
    o<R>(o: (this: O<T>, p: (n: T) => R) => O<R>, p: (n: T) => R): O<R>;
  }

  function of<T>(...args: T[]): O<T> { return null; }

  function map<T, R>(this: O<T>, p: (v: T) => R): O<R> { return null; }
  function filter<T>(this: O<T>, p: (v: T) => boolean): O<T> { return null; }

  var a = of(1, 2, 3).o(map, n => n.toString())
  var b = of(1, 2, 3).o(filter, n => n > 1);

The type of a gets inferred to be O<{}> as the R type parameter is not constrained by the environment which means it gets {} then it is merged with the R inferred from the n => n.toString() which is string. Both candidate Rs are merged to which yields {}.

The type of b is correct in this example as there T is bound in the call to of().

The @IgorMinar proposal is better for map but worse for filter and can be summarized by:

  interface O<T> {
    o<R>(t: (s: O<T>) => O<R>): O<R>;
  }
  function of<T>(...args: T[]): O<T> { return null; }
  function map<T, R>(p: (n: T) => R): (s: O<T>) => O<R> { return null; }
  function filter<T>(p: (n: T) => boolean): (s: O<T>) => O<T> { return null;}

  var a = of(1, 2, 3).o(filter(n => n > 1));
  var b = of(1, 2, 3).o(map(n => n.toString()));

Here the type of a is O<{}> for similar reasons the map example failed above.
The type of b however is right but the type of n is lost and we only get O<string> because toString() is always type string. If the type of the lambda depended on the type of n then we would get this wrong too.

The @Blesh proposal is really an extends pattern and can be typed using the & type operator and summarized by:

interface O<T> {
    o<V>(operators: V): O<T> & V; 
  }

  function map<T, R>(this: O<T>, p: (v: T) => R): O<R> { return null; }
  function filter<T>(this: O<T>, p: (v: T) => boolean): O<T> { return null; }

  function of<T>(...args: T[]): O<T> { return null; }

  var a = of(1, 2, 3).o({map}).map(n => n.toString());
  var b = of(1, 2, 3).o({filter}).filter(n => n > 1);

In this example, the types of all the variables and parameters are what you would expect them to be. a is of type O<string> and b is of type O<number>. However, the projected operators get lost immediately so you cannot string them together as one of the proposal's examples:

var c = of(1, 2, 3).o({filter, map}).filter(n => n > 1).map(n => n.toString())

Here the call to map is flagged as an error as the type of filters result is O<number> not O<number> & {filter: typeof filter, map: typeof map} as you might expect. This makes chaining laborious as the call to o must be repeated at each step.

var c = of(1, 2, 3).of({filter}).filter(n => n > 1).o({map}).map(n => n.toString());

@jayphelps
Copy link
Member Author

jayphelps commented Oct 14, 2016

Talking with @Blesh, he also brought up a pretty neat pattern, using private Symbols.

The operators you use internally can be added in a single place like:

import { Observable } from 'rxjs/Observable';
import { filter } from 'rxjs/operator/filter';
import { map } from 'rxjs/operator/map';
import { switchMap } from 'rxjs/operator/switchMap';

export const $$filter = Symbol('@@filter');
export const $$map = Symbol('@@map');
export const $$switchMap = Symbol('@@switchMap');

Observable.prototype[$$filter] = filter;
Observable.prototype[$$map] = map;
Observable.prototype[$$switchMap] = switchMap;

Then when you want to use them:

import { $$map, $$filter } from './operators';

of(1, 2, 3)
  [$$filter](i => i > 1)
  [$$map](i => i * 10)
  .subscribe(x => console.log(x));

// or if you don't like the $$ Symbol prefix convention

of(1, 2, 3)
  [filter](i => i > 1)
  [map](i => i * 10)
  .subscribe(x => console.log(x));

This also is not (currently) typesafe though as described here.

@benlesh
Copy link
Member

benlesh commented Jan 25, 2017

@jayphelps ... I'm rekindling interest in this, actually. It might be useful in falcor-router to prevent leaking internal implementation details.

@benlesh
Copy link
Member

benlesh commented Jan 25, 2017

@jayphelps ... how do you feel about just having one method, but naming it op()? Terse, but still gets the point accross... "operators" is frequently reduced to "ops" in our field.

@benlesh
Copy link
Member

benlesh commented Jan 25, 2017

Also there's this video: (click it! click it!)

Eep Op Ork Ah Ah

@benlesh benlesh closed this Jan 25, 2017
@benlesh benlesh reopened this Jan 25, 2017
@benlesh
Copy link
Member

benlesh commented Jan 25, 2017

Oops accidentally closed.

@jayphelps
Copy link
Member Author

jayphelps commented Jan 25, 2017

@Blesh I'm down with op()

import { of } from 'rxjs/observable/of';
import { filter } from 'rxjs/operator/filter';
import { map } from 'rxjs/operator/map';

of(1, 2, 3)
  .op(filter, i => i > 1)
  .op(map, i => i * 10)
  .subscribe(x => console.log(x));
// 20..30

Now that's been a little while I'm curious if @IgorMinar and everyone else has different opinions than before, especially related to lack of type safety.

We certainly could require that all operators patch the op() signature with their specific overload to make it type-safe.


Final thought... would it be useful for this method to support partial application of arguments (similar to Function.prototype.bind)?

I'm not sure how that would work in some operators cases... like merge for example... but

source$.op(merge, a$, b$)

I'm not sure I follow? I believe your example would work as-is with this PR, wouldn't it? It doesn't partially apply, it really just proxies (or whatever)

@coveralls
Copy link

coveralls commented Jan 25, 2017

Coverage Status

Coverage increased (+0.002%) to 97.041% when pulling feb379d on jayphelps:operate into 1bee98a on ReactiveX:master.

@benlesh
Copy link
Member

benlesh commented Jan 25, 2017

I'm not sure I follow? I believe your example would work as-is with this PR, wouldn't it? It doesn't partially apply, it really just proxies (or whatever)

I started typing that, then accidentally hit "Close and Comment" when I meant to cancel what I was typing. :) It was a stupid idea.

@benlesh
Copy link
Member

benlesh commented Jan 25, 2017

I think @david-driscoll's idea for adding some type-safety was pretty decent:

Worst case we should be able to augment the operator on Observable inside of each function module. Which would basically be what the signature interfaces were. Except we don't have to worry about augmenting the prototype, overloading the method be sufficient.

Seems viable. Did we after test this?

@david-driscoll
Copy link
Member

Proof of concept using the current playground is available here

Code below

interface Observable<T> {
  op(operator: typeof filter, predicate: (x: T) => boolean): Observable<T>;
}

function filter<T>(o: Observable<T>, predicate: (x: T) => boolean) { }


let o: Observable<string>;
// x is a string here
o.op(filter, x => x === 1);
o.op(filter, x => x === '1');

let o2: Observable<{ a: string; b: number }>;
// x is a string here
o2.op(filter, ({ a, b }) => a === 'hello' && b === 1);

@trxcllnt
Copy link
Member

I'm totally for this feature, but just a quick question: does anybody know if fantasy-land already have a name for this? If so, it would seem valuable to align here.

@staltz
Copy link
Member

staltz commented Jan 28, 2017

@trxcllnt I'm afraid not, since op would be n-ary where n arguments vary depending on the first argument. No such thing in Fantasy Land. To be honest this op looks like Function.call or Ramda's http://ramdajs.com/docs/#call

@benlesh
Copy link
Member

benlesh commented Jan 29, 2017

@mhegazy ... do you see any compile-time performance problems with @david-driscoll's proposal above? Basically we'd be looking at doing this with 60+ operators, some of which have 6-7 overloads.

Allows you to apply an operator to a given stream, without it needing to
be
on the Observable prototype. This is useful whenever you don't want to
unintentionally leak implementation details or otherwise rely on the
fact
that you're using a particular operator.

It takes the provided operator function and any arbitrary arguments
then internally just calls `operator.apply(this, args)`.

This is solves a similar problem as the TC39 proposed [bind operator
syntax](https://github.com/tc39/proposal-bind-operator)
e.g. `stream::map(i => i + 1)` but that's stage-0 and not supported by
TypeScript.

Example 1: in your unit tests, if you were to patch additional operators
onto the Observable prototype then the application you're testing could
unknowingly be relying on those operators without it importing them
itself.
Your tests would pass because the operator is available in your tests,
but
when you run the app standalone it errors because the operator wasn't
imported!

Example 2: you're writing a library that itself uses RxJS. If you patch
operators onto the prototype, then consumers of your library could
accidentally depend on the fact that the operators you use are
available.
@rxjs-bot
Copy link

rxjs-bot commented Feb 8, 2017

Fails
🚫 missing type definition import in tests (Observable-spec.ts) (1)

(1) : It seems updated test cases uses test scheduler interface hot, cold but miss to import type signature for those.

Generated by 🚫 dangerJS

@jayphelps
Copy link
Member Author

don't believe this PR is applicable anymore

@jayphelps jayphelps closed this Jun 15, 2017
@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