rxjs: subscribing fails with a function that doesn't have a normal `bind()`

[Update: the conversation has shifted down-thread, from this OP being about a “broken” subscriber callback, to whether or not there’s any merit to being able to opt-out of bind(..)ing, for performance reasons]

Describe the bug

I have a unique situation where I’m trying to subscribe to an observable with a function that has an overridden bind(..), which does something different than normal Function#bind would do. I tried passing that function as a subscription next function, and subscribe(..) either seems to ignore it (silently) or in some other way just fails to ever call it as expected.

Expected behavior

I expected to be able to pass in any function that is normally callable, like fn(..) (or fn.call(..) or fn.apply(..)) and have it work fine as a subscription.

As shown in the reproduction code below, the simple workaround is to wrap my function in an arrow/lambda. That’s not awful, but it’s annoying and a bit wasteful to have to do so. I’d really like to avoid that.

I wouldn’t have expected that subscriber functions must be this-bindable. I read through some documentation and I didn’t see such a gotcha listed – apologies if this is documented and I missed it.

If it’s not documented, I think it probably should be!

My argument against the current behavior is, a lot of people probably pass => functions anyway, so Rx is likely not getting much out of trying to bind the this context on those subscription function calls.

But if that behavior is just absolutely required by RxJS, I wonder if there could be some way of opting out that doesn’t involve having to wrap my function in an arrow/lambda every time? Perhaps, for example, it could be something like one of these:

rx.subscribe({ _next: myA });   // or some other variation of property name

// or
rx.subscribe({ next: myA, noBind: true });

// or
rx.subscribeNoBind({ next: myA });

It could ALSO possibly be that having a path to opt-out of this binding – for those like me don’t need or want it – might even be just ever so slightly faster on execution. 😉

Reproduction code

import { of } from 'rxjs';

function myA(v) { console.log("a",v); }
function myB(v) { console.log("b",v); }

myA.bind = function(){ /*.. something different ..*/ }
myB.bind = function(){ /*.. something different ..*/ }

const rx = of(42);

// does not work
rx.subscribe({ next: myA });

// works fine
rx.subscribe({ next: v => myB(v) });

Reproduction URL

https://stackblitz.com/edit/rxjs-y1wqko

Version

7.5.2

Environment

Node 16

Additional context

The source of these special functions (that don’t have a normal behaving bind(..)) is from a library I wrote, and I want users to be able to use them with RxJS as easily as possible.

About this issue

  • Original URL
  • State: closed
  • Created 2 years ago
  • Comments: 19 (12 by maintainers)

Commits related to this issue

Most upvoted comments

@benlesh I’m reluctant to indulge your actual question directly, as its focus on my previous-but-now-set-aside motivations may further distract and invite bikeshedding of its own.

However, for curiosity/posterity sake, I will explain the kind of functions that led me to even discover that Rx was doing this binding – I never knew it did that in all my years interacting with the lib! But I’m gonna put the rest of my answer below, so it’s not as tempting to get off into the weeds any further.

The way I discovered that Rx does this binding of subscribe callbacks is by providing a function to Rx that intentionally has replaced the normal bind(..) function with one that does a very different kind of task to this binding. This kind of function will never be this-aware and therefore has absolutely no reason for this-binding. It’s also a unary function, so it would never need the partial-application capabilities that built-in bind(..) provides.

So why did I override the name bind(..) instead of picking a different method name? There’s a very strong reason to do so. Trust me, I didn’t make the decision lightly. I know the downsides.

The special kind of functions I’m talking about are created by my library: Monio. Monio is a monads-in-JS library.

As you may be aware, the concept of monads has one central capability associated with it, which often goes by one of several very common names in the FP world: bind(..), chain(..), or flatMap(..). Regrettably, there’s no standard name for this behavior, so depending on your experiences, you may be used to or prefer any of those 3 (or something else entirely!) My informal surveying estimates that more than 90-95% of all monad implementations, especially in JS, use one of those 3 names.

Monio chose, 2+ years ago when I first wrote it, and in an attempt to be as widely approachable as possible, to use all 3 names as aliases, so you can pick whichever your favorite name is for this core monadic behavior.

I didn’t ever anticipate at the original time of design that the monad instances created by Monio would be anything other than normal objects, so the method names I put on it were never subject to a collision issue. However, I knew that bind(..) and flatMap(..) were both commonly used method names in other parts of JS, such as on functions and arrays.

Well, as it so happens, a few months ago, I designed a new monad kind for Monio, and these instances of this monad kind are… functions. Not monads holding a function as a value, but literally the monad instance itself is a function.

Now of course, I have a collision issue between monadic bind(..) and JS function’s bind(..). What to do!?!?

  1. Do I break all of Monio usage by removing bind(..) from all monad kinds? It’s far too late for that sort of breaking change, given that Monio has been around (and used in production) for 2+ years.
  2. Do I leave these new functions having the built-in bind(..) method? That’s problematic if someone is used to using the name bind(..) with all other monad kinds in Monio, and then for this new kind the bind(…)` method isn’t what they expect!
  3. Do I delete the bind(..) method name entirely? Tempting, since these functions-as-monad-instances will never need this-binding or partial application. But again, creates the same problem for those using bind(..) on other monad kinds and not being able to use bind(..) on this kind of monad.
  4. Do I overwrite the bind(..) method to alias the monadic bind? Seems like the more appropriate move for Monio and its users. For completeness sake, I actually back-up the original bind(..) as _bind(..) on the function, just in case in some extraordinary case some really had to have the “real” bind.

OK, so option 4 is what I chose. It’s not perfect, but it’s better than (2) and (3) which create an inconsistency. And (1) is a really extreme route that I don’t wanna do.

Here’s where things get more interesting… the new monad kind I’ve invented for Monio is a special kind of monad that is basically mimicking, or acting as a companion to… observables. They’re called IOx (as in “Reactive IO”). In fact, I have a fromObservable(..) method on this new monad kind, which is designed to consume an Rx observable and turn it into an IOx, and of course the other direction too, toObservable(..) is an obvious and useful tool.

My point? These IOx monads are actually designed and intended that people will use them interactively with Rx observables in their code base. So it’s very likely that one of them could end up being used as a subscription callback to Rx.

Writing tests for my library is exactly how I found this current incompatibility. It’s obviously quite ironic that in trying to make my library more friendly to Rx users, I discovered this gotcha that makes them ultimately somewhat-less compatible.

So then I started wondering if there was some way for Rx to add a small additional opt-out feature, which would smooth over this incompatibility. As I illustrated above, it’s not really an option for Monio to rename its methods or create inconsistencies in its design.

Ultimately, if this request is denied, it’ll just make it a little more error prone for folks to use my library with Rx. That bums me out, but it’s not the end of the world.

+4
getify on Feb 5, 2022

FWIW: This issue has me thinking about a better architecture in general for Subscriber going forward, and I’m pretty excited about it. But I think it’s going to be something we’ll have to try to get to in version 8, not version 7.

+3
benlesh on Feb 7, 2022

Just to re-hash in the simplest terms. Whatever bind calls are done in RxJS v7, are done specifically so we don’t break this on supplied object methods when we try to wrap the functions to deal with unhandled errors (an RxJS feature). Basically just ensuring this works:

source$.subscribe({
  next(value) {
     this.myMethod(value);
  },
  myMethod(value) {
     console.log(value);
  }
})

The this.unsubscribe() stuff is deprecate and a red herring.

However, what is being posed as a bug/issue here is something that would break with even the simplest function that relied on Function.prototype.bind’s behavior:

function logThisMessage() {
   console.log(`Message`, this.message);
}

// This is going to break everything.
logThisMessage.bind = () => { /* lol */ };

function useHiThere(fn) {
  return fn.bind({ message: 'hi there' });
}

useHiThere(logThisMessage)();

So I guess what I’m going to ask, @getify, can you present a case why it’s reasonable for any library to assume that users have passed in a function with a broken bind method? I don’t mind the fix, honestly, I just want to make sure we’re not accommodating a corner-case we shouldn’t be.

+3
benlesh on Feb 4, 2022

@getify FYI: the fix for this is published with 7.5.3

+2
benlesh on Feb 8, 2022

I actually have two different PRs for different approaches to this #6796 and #6797.

That said, this is a highly unusual ask. Basically, @getify, you’re saying “please don’t rely on the behavior of Function.prototype.bind because someone could patch it”. Given that Function is one of the most primitive types in JavaScript, if we can’t rely on its methods, I’m not sure what we can rely on.

I’m not sure what the performance implications are for either of my PRs. I feel like the second one is closest to parity with the original code, however the first one with the arrow functions will be cleaner in the 8.x branch (where we’ve done away with a deprecated behavior related to the whole bind thing).

+2
benlesh on Feb 4, 2022

It’s really not required for anything. It’s one approach to ensuring we don’t break what this means inside of each of the functions passed with the observer. Short-term one of the two PRs I have would do. Longer-term, I’d like to re-architect things in this area to help perf a little. To that end, I have some local spikes for version 8.

Anyhow, this all requires careful consideration. I would rather not introduce new features, as we end up supporting them for years after. Ideally any solution would “just work” and be generally beneficial for the community.

+1
benlesh on Feb 6, 2022

To say this is user facing, in my opinion I believe there should be feasible user story that this can break their application code when code follows spec.

Whether it’s documented or not, the fact that every single subscriber callback is this-bound to the subscriber, makes it a user-facing feature. There must be code out there that is relying on that, and will break if Rx removed bind(..) internally for some reason.

In fact, the code comment I referenced above, which talks about intentionally deprecating the access to this.unsubscribe(..), is proof that this is considered a “feature”; if it was only an implementation detail, there’d be no need to deprecate it.

I think I have expressed my own opinion enough

Me too. I think this should be considered as a feature-request. But Rx isn’t my library. I don’t need to spend any more time advocating for it. If y’all decide to reject the request, c’est la vie. Sorry if I’ve been disruptive here.

+1
getify on Jan 29, 2022
  1. I do not think we can skip this request is coming from your code’s specific usecases. I still think this is very specific to implementation detail of a library.
  2. As I already mentioned, we may, or may not decide to stop using bind, but I do not believe we’ll explicitly exposing it as public level.
  3. I do not agree proposed api surface exposure, again, in my opinion this is very close to implementation detail. First, what’ll happen if we internally decide to not to use bind at all? What happens in public api surface? Also, what’ll happen if we choose other mechanism which potentially bring similar questions? What is actual performance overhead we are currently seeing? Do we have proven performance overhead worth enough to expose additional api surface? Especially given general direction where RxJS attempts to proceed is reduce & simplify api surfaces as much.

Overall, unless I miss some points this request asks to change internals of library, or expose those internals of libaray into public api surface without noticeable gain we can actually achieve. I’d happy to be proven wrong if I could see 1. bind actually creates lot of overhead 2. Therefore, we’d like to remove it in the future version but in any case I don’t think I’ll agree to expose those into public api surface level.

+1
kwonoj on Jan 29, 2022

Other core team members might have a different opinion, but in my opinion, this is internal implementation detail of a, or any library that relies on standard behavior of javascript. Yes, as you said bind itself may have slightly different characteristics other than some spec behavior of fn - but that doesn’t mean any code / library should be able to provide an escape hatch if consumer intentionally choose to override its behavior to not follow intended specs. I’m a bit lost to understand why RxJS should provide an escape hatch, or explicitly explain it relies on certain spec behavior internally for these cases.

It may possible further version of RxJS could remove usage of bind for some other reasons. If that happens, that may resolve your usecases but I don’t expect we’ll refactor code specifically because of this reason.

+1
kwonoj on Jan 29, 2022
Read more comments on GitHub
← rxjs: Subject.prototype.next() should only accept T, not T | undefined
rxjs: Subscription to Observable is not removed from observers array after unsubscribe() →