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

Rx.Observable.from does not properly handle NodeList #3041

Closed
Dorus opened this issue Nov 3, 2017 · 13 comments
Closed

Rx.Observable.from does not properly handle NodeList #3041

Dorus opened this issue Nov 3, 2017 · 13 comments

Comments

@Dorus
Copy link

Dorus commented Nov 3, 2017

Consider the following code:

const buttons = document.querySelectorAll('.btn');

const button$ = Rx.Observable.from(buttons);
const button2$ = Rx.Observable.from(buttons);

button$.subscribe(x => console.log('observer 1 ', x)); // emit
button$.subscribe(x => console.log('observer 2 ', x)); // nothing
button2$.subscribe(x => console.log('observer 3 ', x)); // emit

Rx.Observable.from should result in a cold observable that emit the same list each time it is subscribed. However, in this example the source is a NodeList and the second subscription does not emit.

@kwonoj
Copy link
Member

kwonoj commented Nov 3, 2017

does fromObservable suppose to return cold explicitly? I thought it depends on source type, like from(Promise) you won't get cold suscribes into new promise each time automatically.

@Dorus
Copy link
Author

Dorus commented Nov 3, 2017

As far as i know promises are eager so it's not even possible for from(Promise) to be anything other than cold. Only fromPromise(() => Promise) is able to rerun the Promise.

@david-driscoll
Copy link
Member

Makes me think that for NodeList (and friends) if it finds the Iterator it's pulling that out. Not 100% sure on that however.

@kwonoj
Copy link
Member

kwonoj commented Nov 3, 2017

I think my main question in here is this: does from explicitly intended to return cold? I think I didn't have indication around those. Dealing with NodeList from static creation seems separate discussion.

@Dorus
Copy link
Author

Dorus commented Nov 3, 2017

Interesting enough, from does reread the source. I stand corrected:

var ar = [1,2,3];
const ar$ = Rx.Observable.from(ar);
ar$.subscribe(x => console.log('observer 1 ', x)); // 1,2,3

ar[2] = 5
ar$.subscribe(x => console.log('observer 2 ', x)); // 1,2,5

@cartant
Copy link
Collaborator

cartant commented Nov 3, 2017

Regarding the iterator, in stable, it's obtained in the constructor. However, in master, it's obtained in _subscribe. So the next major version will behave as the OP is expecting.

@kwonoj
Copy link
Member

kwonoj commented Nov 3, 2017

I just came to remind this, is this related with #2497 ?

@kwonoj
Copy link
Member

kwonoj commented Nov 3, 2017

oh @cartant already pointed it.

@Dorus
Copy link
Author

Dorus commented Nov 3, 2017

I think i actually prefer from to reread the source, that's quite neat. So all we have to do is wait for the next major.

My question is answered so it can be closed for me, unless anyone has another reason to keep it open?

@kwonoj
Copy link
Member

kwonoj commented Nov 3, 2017

@Dorus let me close issue, and sorry for bringing confusion. I did misread original issue obviously and caused confusions around.

@kwonoj kwonoj closed this as completed Nov 3, 2017
@cartant
Copy link
Collaborator

cartant commented Nov 3, 2017

@Dorus You most likely already know this, but you can work around the problem in version 5 using defer:

const button$ = Rx.Observable.defer(() => Rx.Observable.from(buttons));

@Dorus
Copy link
Author

Dorus commented Nov 3, 2017

@cartant Yes i did know that, but thanks for the suggestion anyway. It's exactly what i suggested @mocheng on gitter 3 november 2017 12:14 as he's the one that came up with this scenario.

@kwonoj No prob about the confusion, at least it kept the issue active and quickly resolved :)

@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

No branches or pull requests

4 participants