ReactiveX / learnrx

A series of interactive exercises for learning Microsoft's Reactive Extensions Library for Javascript.
1.4k stars 292 forks source link

Exercise 29 - Wrong explanation #160

Closed schrej closed 5 years ago

schrej commented 7 years ago

In Exercise 29 there is a wrong explanation I think. In this line it states that Observable.fromEvent() returns a Subscription. Doesn't it return an Observerable who's forEach() method returns a Subscription that can be disposed of?

Correct me if I'm wrong please but that's how i thought it works.

gruppjo commented 7 years ago

Think so too. It should probably be forEach() instead of fromEvent():

Notice that Observable's forEach() function returns a Subscription object.

sjames1958gm commented 6 years ago

Doesn't forEach actually return a Promise, because I get an error in the console:

VM1673:12 Uncaught (in promise) TypeError: subscription.dispose is not a function
    at eval (eval at <anonymous> (eval at go.onclick (main.js:81)), <anonymous>:12:18)
    at SafeSubscriber._next (Rx.js:864)
    at SafeSubscriber.__tryOrUnsub (Rx.js:571)
    at SafeSubscriber.next (Rx.js:518)
    at Subscriber._next (Rx.js:459)
    at Subscriber.next (Rx.js:423)
    at HTMLButtonElement.handler (utils.js:92)
schrej commented 6 years ago

Seems to be fixed now, closing this.

seanpoulter commented 5 years ago

Would you re-open this issue @schrej? There's no error in the UX but the error that @sjames1958gm shared appears on the console because forEach returns a Promise. image

Here's the code from Exercise 29:

function(button) {
    var buttonClicks = Observable.fromEvent(button, "click");

    // In the case of an Observable, forEach returns a subscription object.
    var subscription =
        buttonClicks.
            forEach(function(clickEvent) {
                alert("Button was clicked. Stopping Traversal.");

                // Stop traversing the button clicks
                subscription.dispose();
            });
}

Since we're trying to teach functional and reactive programming, what's our best alternative for forEach?

CC: Have you discussed options with the team @morenoh149?

seanpoulter commented 5 years ago

I'd propose changing the exercise to:

function(button) {
    var buttonClicks = Observable.fromEvent(button, "click");

    // In the case of an Observable, forEach returns a subscription object.
    var subscription =
        buttonClicks.
-           forEach(function(clickEvent) {
+           do(function(clickEvent) {
                alert("Button was clicked. Stopping Traversal.");

                // Stop traversing the button clicks
-               subscription.dispose();
+               subscription.unsubscribe();
-           })
+           }).
+      subscribe();
}
morenoh149 commented 5 years ago

@seanpoulter if you make it a PR I'd merge it.

seanpoulter commented 5 years ago

Thanks for the quick reply @morenoh149! 🎉 The PR is open.