feat(from): allow Observable.from to handle array-like objects #1195

justinwoo · 2016-01-15T11:15:16Z

per #1166, this brings the signature and functionality closer to Observable.from
that is in RxJS4.

Cover your eyes, folks, this ain't pretty. Would appreciate feedback on how this should be changed. (e.g. should the default signature just be the 4-arg one with the mapFn? should fromArray just do the mapFn stuff by default and merge definitions with this? etc.)

david-driscoll · 2016-01-15T13:33:05Z

src/util/isArrayLike.ts

@@ -0,0 +1 @@
+export const isArrayLike = ((x: any): boolean => x && typeof x.length === 'number');


You can make this a type guard, so TS can better infer types inside the if block;

export const isArrayLike = (<T>(x: any): x is ArrayLike<T> => x && typeof x.length === 'number');

(edit: @Blesh ... updated code block format to TypeScript)

The extra parens around both of these examples confused me.

Also, protip: in Github if you start with TypeScript it will format the code block as TypeScript.js for JavaScript, etc. ```ts might work.

benlesh · 2016-01-15T18:48:47Z

Couldn't this have been as easy as adding:

if (isArrayLike(ish)) {
  return new ArrayObservable(Array.from(ish.length), mapFn);
}

justinwoo · 2016-01-16T00:30:10Z

Right, could be just as simple as return new ArrayObservable(Array.from(ish.length, mapFn, thisArg), scheduler);, but that does front-load transforming the values of the array-like into the transformed Observable, right?

If there's nothing about the scheduler semantics that needs to be worked out, then that would be fine with me, but I was delaying the transformation of the values until they are called for either on subscription or scheduler dispatch.

justinwoo · 2016-01-16T05:19:24Z

Updated based on feedback, but would love to hear if I can just get rid of most of this code or just merge this into the normal ArrayObservable and change the from signature entirely.

luisgabriel · 2016-01-16T05:21:57Z

src/observable/from.ts

+  static create<T>(ish: any, mapFnOrScheduler: Scheduler | ((x: number, y: any) => T), thisArg?: any, lastScheduler?: Scheduler): Observable<T> {
+    let scheduler: Scheduler;
+    let mapFn: (x: number, y: any) => T;
+    if (typeof mapFnOrScheduler === 'function') {


You can use isFunction() here.

luisgabriel · 2016-01-16T05:40:19Z

What about this:

//import {map} from '../operator/map'
if (isArrayLike(ish)) {
  const source = new ArrayObservable(Array.from(ish.length), scheduler);
  return map.call(source, mapFn, thisArg);
}

benlesh · 2016-01-16T09:45:26Z

from should have a result selector we can use. If it doesn't, we should add it.

justinwoo · 2016-01-16T14:40:33Z

Result selector for every kind of input so that we use the same method signature as rxjs4 observable.from?

benlesh · 2016-01-18T18:28:51Z

Actually, I've thought it over. This is the proper implementation. Disregard my silly idea above, you don't want to allocate another array with Array.from.

benlesh · 2016-01-18T18:29:50Z

spec/observables/from-spec.js

+      expect(x).toBe(expected[i++]);
+    }, null, done);
+  });
+


Can we add a test for arguments? I suspect this will be a primary use case for this, since it's the most common ArrayLike.

Added on L30 above.

justinwoo · 2016-01-20T03:36:07Z

Updated and all based on feedback. Are we okay with the fromArray/fromArrayLike split?

kwonoj · 2016-01-21T22:58:38Z

src/util/isArrayLike.ts

@@ -0,0 +1 @@
+export const isArrayLike = (<T>(x: any): x is ArrayLike<T> => x && typeof x.length === 'number');


do we have common usage's of this function over codebase except from? if not, what about defer to export it once there's usage?

I'd rather not crowd from with barely related defs and end up having split history.

if it contains multiple lines I'd definitely agree, in this case would like to follow suggestion in #1211 (comment) , similar case to this.

If you insist. Pushed it up now.

It wasn't trying to insist, apologizes for my tone makes you felt.

It's fine, I'm just biased towards separating it out.

kwonoj · 2016-01-22T02:29:33Z

Change looks good to me, let me leave PR opened bit more to see if any other suggestions around.

this brings the signature and functionality closer to `Observable.from` that is in RxJS4.

kwonoj · 2016-01-31T08:06:59Z

Merged with 7245005, thanks @justinwoo .

lock · 2018-06-07T12:02:40Z

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.

david-driscoll reviewed Jan 15, 2016
View reviewed changes

justinwoo force-pushed the from-arraylike branch from 0320925 to 8e9dc97 Compare January 16, 2016 05:18

luisgabriel reviewed Jan 16, 2016
View reviewed changes

justinwoo force-pushed the from-arraylike branch from 8e9dc97 to 85cc8af Compare January 16, 2016 06:48

benlesh reviewed Jan 18, 2016
View reviewed changes

justinwoo force-pushed the from-arraylike branch from 85cc8af to 2c265f2 Compare January 18, 2016 23:22

kwonoj reviewed Jan 21, 2016
View reviewed changes

justinwoo force-pushed the from-arraylike branch 2 times, most recently from 90714da to e0f47b3 Compare January 22, 2016 02:24

kwonoj added the PR: lgtm label Jan 22, 2016

feat(from): allow Observable.from to handle array-like objects

8bde821

this brings the signature and functionality closer to `Observable.from` that is in RxJS4.

justinwoo force-pushed the from-arraylike branch from e0f47b3 to 8bde821 Compare January 26, 2016 02:38

justinwoo mentioned this pull request Jan 28, 2016

Should we add a resultSelector to Observable.from? #1255

Closed

kwonoj closed this Jan 31, 2016

lock bot locked as resolved and limited conversation to collaborators Jun 7, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(from): allow Observable.from to handle array-like objects #1195

feat(from): allow Observable.from to handle array-like objects #1195

justinwoo commented Jan 15, 2016

david-driscoll Jan 15, 2016

benlesh Jan 15, 2016

benlesh commented Jan 15, 2016

justinwoo commented Jan 16, 2016

justinwoo commented Jan 16, 2016

luisgabriel Jan 16, 2016

luisgabriel commented Jan 16, 2016

benlesh commented Jan 16, 2016

justinwoo commented Jan 16, 2016

benlesh commented Jan 18, 2016

benlesh Jan 18, 2016

justinwoo Jan 18, 2016

justinwoo commented Jan 20, 2016

kwonoj Jan 21, 2016

justinwoo Jan 22, 2016

kwonoj Jan 22, 2016

justinwoo Jan 22, 2016

kwonoj Jan 22, 2016

justinwoo Jan 22, 2016

kwonoj commented Jan 22, 2016

kwonoj commented Jan 31, 2016

lock bot commented Jun 7, 2018

		@@ -0,0 +1 @@
		export const isArrayLike = ((x: any): boolean => x && typeof x.length === 'number');

		@@ -0,0 +1 @@
		export const isArrayLike = (<T>(x: any): x is ArrayLike<T> => x && typeof x.length === 'number');

feat(from): allow Observable.from to handle array-like objects #1195

feat(from): allow Observable.from to handle array-like objects #1195

Conversation

justinwoo commented Jan 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benlesh commented Jan 15, 2016

justinwoo commented Jan 16, 2016

justinwoo commented Jan 16, 2016

Choose a reason for hiding this comment

luisgabriel commented Jan 16, 2016

benlesh commented Jan 16, 2016

justinwoo commented Jan 16, 2016

benlesh commented Jan 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinwoo commented Jan 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kwonoj commented Jan 22, 2016

kwonoj commented Jan 31, 2016

lock bot commented Jun 7, 2018