Rename sample operators to inspect, reimplement sample operator

[benlesh]

Per some offline discussion with @headinthebox, @mattpodwysocki and @staltz... the current implementation of sample as discussed in #150 is probably incorrect.

Basically, in the original sample operator ala RxJS 4, the emission of a value meant that a new value had arrived on the source. If the notifier notified, and a new value hadn't arrived, nothing would be emitted. This means the very emission of the value has some meaning/connotations WRT the source observable.

... that said ... I think @staltz raised some interesting points about how people might expect a sample operator to work, or needs those people might have.

... so rather than throw away the current sample and sampleTime implementations, I'd like to rename them to inspect and inspectTime and then reimplement the sample operators in RxJS 4's likeness.

[staltz]

I'd question the usefulness of both inspect and sample flavors, since we're just adding more operators.

It seems easier to keep in core only radically important operators (like mergeMap, scan, map, take...), than to bloat core with many operators.

[staltz]

@Blesh There's also the issue with sample/inspect that the completion of the sampler Observable should "sample" the sampled Observable, like Erik mentioned. That might be important for beta.

[benlesh]

There's also the issue with sample/inspect that the completion of the sampler Observable should "sample" the sampled Observable, like Erik mentioned. That might be important for beta.

👍 good catch. I don't think I included this in my PR. I'll add that.

[benlesh]

I'd question the usefulness of both inspect and sample flavors, since we're just adding more operators.

As long as inspect is in KitchenSink, and we're going with this modular design. I don't think it matters.

If it came down to removing one, though, I'd remove inspect. Only because it could be composed from sample, where the reverse isn't really possible.

[staltz]

I'm curious, how would inspect be written using sample and others?

[benlesh]

Thinking quickly: Something like... notifier.withLatestFrom(source.sample(notifier), (_, a) => a)? That might not be perfect, but it's an approximation, I'd guess. The emitting on completion behavior of sample might make it weird, but you could compose around that too.

[staltz]

Why not just notifier.withLatestFrom(source, (_, a) => a)?

[benlesh]

Why not just notifier.withLatestFrom(source, (_, a) => a)?

I don't know? lol.

[staltz]

Seriously, I could run all inspect tests replaced with withLatestFrom. If everything works fine, we could remove inspect.

[staltz]

Just FYI, withLatestFrom needs also the addition of last() in order to get the completion semantics (almost) right:

  it('should get inspections when the notifier emits', function () {
    var e1 =   hot('----a-^--b----c----d----e----f----|          ');
    var e1subs =         '^                           !          ';
    var e2 =   hot(      '-----x----------x----------x----------|');
    var e2subs =         '^                           !          ';
    var expected =       '-----b----------d----------f|          ';

    var result = e2.withLatestFrom(e1, function (a, b) { return b; }).takeUntil(e1.last());

    expectObservable(result).toBe(expected);
    expectSubscriptions(e1.subscriptions).toBe(e1subs);
    expectSubscriptions(e2.subscriptions).toBe(e2subs);
  });

I don't know how useful it is to have the completion semantics of inspect, but since we just introduced this operator, I suppose people don't depend on how inspect works. For that reason it's probably better to kill inspect and recommend instead withLatestFrom if anyone needs something similar to inspect. Just my opinion.

[headinthebox]

Agreed.

[benlesh]

I'm fine with that.

[lock]

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.