fix(Observable.from): use scheduler if supplied

[chrisprice]

Description:

Arguments supplied to Observable.from are not being propagated correctly. I've chased this a couple of levels down the rabbit hole and whilst this patch fixes some issues, I'm starting to think that Iterators and Schedulers just don't work together. I supply this patch only because the code is so obviously wrong.

N.B. I tried adding tests but I don't understand enough about your testing primitives and their behavior to get the results I would expect. However, I believe the code is currently so obviously wrong that the tests are unnecessary to prove correctness, only to prevent regression.

[coveralls]

Coverage increased (+0.03%) to 96.51% when pulling 04637ff on chrisprice:fix-observable-from into 57d64cb on ReactiveX:master.

[benlesh]

Yeesh. Good catch, @chrisprice . Do you think you can add a test that verifies the change? (i.e. one that was broken before the change, but works now?)

[chrisprice]

This is the rabbit hole I was talking about 😬 The following test will pass -

  it('should handle any iterable thing with a scheduler', (done: MochaDone) => {
    const iterable = <any>{};
    const iteratorResults = [
      { value: 'one', done: false },
      { value: 'two', done: false },
      { done: true }
    ];
    const expected = ['one', 'two'];

    expect($$iterator).to.equal(Symbol.iterator);

    iterable[Symbol.iterator] = () => {
      return {
        next: () => {
          return iteratorResults.shift();
        }
      };
    };

    Observable.from(iterable, x => x, null, Rx.Scheduler.async).subscribe((x: string) =>  {
      expect(x).to.equal(expected.shift());
    }, (x) => {
      done(new Error('should not be called'));
    }, () => {
      expect(expected.length).to.equal(0)
      done();
    });

    expect(expected.length).to.equal(2);
  });

But will fail if the following line is substituted -

    Observable.from(iterable, Rx.Scheduler.async).subscribe((x: string) =>  {

The conditionals around the arguments seems very inconsistent across both the create function and the constructors invoked by it. E.g. what combinations of arguments are supported? How should arguments be tested? What happens to the mapFnOrScheduler and thisArg arguments for non-IteratorObservables or ArrayLikeObservables?

I don't mind chasing a few of these down but are you able to give some some guidance around the API you're aiming for?

[benlesh]

If something takes a scheduler, it should be an optional last argument. If the arguments before the scheduler are optional, then I'd expect any order would work:

• object
• object, scheduler
• object, fn
• object, fn, scheduler
• object, fn, object
• object, fn, object, scheduler

Should all work.

[chrisprice]

I've modified the from-spec suite of tests to run a standardised set of expectations for all of the possible Observable sources. Here are the results (after 04637ff) -

name	object	object, scheduler	object, fn	object, fn, scheduler	object, fn, object	object, fn, object, scheduler
observable	✅	✅	❌	❌	❌	❌
array	✅	✅	❌	❌	❌	❌
promise	✅	✅	❌	❌	❌	❌
iterator	✅	❌	❌	❌	❌	❌
array-like	✅	✅	✅	❌	✅	✅
string**	✅	❌	✅	❌	✅	✅
arguments**	✅	❌	✅	❌	✅	✅

** Implements iterator protocol so results duplicate iterator.

Before I spend any time fixing all the ❌s, is it worth reconsidering whether the projection and context arguments are useful? I would expect arrow-functions and map negate the need for them?

Updated, I'd made a mistake transcribing the table

[chrisprice]

Apologies for the multiple edits on the table, these are the results which I believe are now accurately transcribed above (with --bail option turned off) -

$ npm run build_test | grep Observable.from
  1) Observable.from should accept observable and projection:
  2) Observable.from should accept observable, projection and scheduler:
  3) Observable.from should accept observable, projection and context:
  4) Observable.from should accept observable, projection, context and scheduler:
  5) Observable.from should accept array and projection:
  6) Observable.from should accept array, projection and scheduler:
  7) Observable.from should accept array, projection and context:
  8) Observable.from should accept array, projection, context and scheduler:
  9) Observable.from should accept promise and projection:
  10) Observable.from should accept promise, projection and scheduler:
  11) Observable.from should accept promise, projection and context:
  12) Observable.from should accept promise, projection, context and scheduler:
  13) Observable.from should accept iterator and scheduler:
  14) Observable.from should accept iterator and projection:
  15) Observable.from should accept iterator, projection and scheduler:
  16) Observable.from should accept iterator, projection and context:
  17) Observable.from should accept iterator, projection, context and scheduler:
  18) Observable.from should accept array-like, projection and scheduler:
  19) Observable.from should accept string and scheduler:
  20) Observable.from should accept string, projection and scheduler:
  21) Observable.from should accept arguments and scheduler:
  22) Observable.from should accept arguments, projection and scheduler:

[benlesh]

@chrisprice I think you raise a valid question. I know that the idea of a projection function as an argument to Observable.from was shot down in @zenparsing's spec. Perhaps we should put this PR on hold until the community has time to discuss whether or not we really want to keep it in there. FWIW, I've never once used the projection argument to from. Generally, I think that Observable.from(whatever).map(project) is more idiomatic and easier to read. I also think we could work on collapsing things like that map so there isn't a performance concern there.

[coveralls]

Coverage increased (+0.2%) to 96.71% when pulling f77e9ca on chrisprice:fix-observable-from into c76a14d on ReactiveX:master.

[coveralls]

Coverage increased (+0.2%) to 96.71% when pulling f77e9ca on chrisprice:fix-observable-from into c76a14d on ReactiveX:master.

[benlesh]

Where did this test go? (Also, I think this test was invalid)

[chrisprice]

Folded into the matrix test below. I can restore it but I think it would just be a dupe.

[chrisprice]

I think I've addressed all the comments with the latest update -

• Removed all default values from constructors
• Added an explicit test for observable-like to the coverage matrix

[coveralls]

Coverage increased (+0.2%) to 96.71% when pulling 553c5bb on chrisprice:fix-observable-from into 4f65b03 on ReactiveX:master.

[jayphelps]

@Blesh how's this look besides needing rebase?

[benlesh]

LGTM

[jayphelps]

@chrisprice just needs a rebase then it's merge time 🎉

[coveralls]

Coverage increased (+0.2%) to 97.068% when pulling 3f052a6 on chrisprice:fix-observable-from into 3c35b94 on ReactiveX:master.

[chrisprice]

Rebased on to 3c35b94 👍

[jayphelps]

Thanks !!

[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.