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

[getify]

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

[kwonoj]

I'm not sure if I understood this correctly.

So, this occurs

1. A function deliberately override prototype.bind which makes a function does not behave as documented spec behavior of prototype.bind
2. and if observable receives those as one of observer callback it will not work as observable internally relies on prototype.bind.

Am I reading this correct? Given observer's callback requires a function, I would say it implicitly requires given function should behave as function which is spec behavior for any of prototype behavior. I may miss something but didn't get fully why bind needs to be treated specifically.

[getify]

Am I reading this correct?

Yes.

I would say it implicitly requires given function should behave as function

It's a normal function that's fully callable in all ways (fn(), new fn(), fn.call(..), fn.apply(..)). It's just not a function that can vend a new this-bound version of itself via fn.bind(..).

note: arrow functions (which are extremely common in JS) are also not bindable... it's just that bind(..) exists on arrow functions and is a wasted no-op. For my library's functions, the bind(..) is overridden to do something different.

What I'm perplexed about is why RxJS needs to be able to bind such functions anyway?

But, if Rx really needs to be able to do so for some use-cases, is there an affordance that Rx can offer so users can opt out to avoid the this-binding part... that wouldn't even have to be default, so no breakage of existing code.

And bonus: if you opt-out and skipped the this-binding of your subscription, that likely would perform ever-so-slightly more efficiently.

[kwonoj]

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.

[getify]

but I don't expect we'll refactor code specifically because of this reason

If you like, forget for a moment that it's a "breakage" I'm experiencing. Pretend my request is an additional feature purely from the justification that I want to skip the "unnecessary" (albeit very slim) performance overhead of bind(..) being called on my function, when I know my function is not this-aware and therefore the this-binding is pointless.

I'm a bit lost to understand why RxJS should provide an escape hatch

Anyone who passes => arrow functions in (I suspect a lot of devs!) can now opt-out of pointless bind(..)ing and may trim a dozen microseconds off each subscribe/dispatch.

---

SIDE NOTE: As I was poking around in the code to propose what changes would be necessary, I found this note which seems to indicate that at least part of the reason for this binding of subscription next handlers was for a now deprecated access to this.unsubscribe(..)... a feature that's now behind a flag and going to be removed (in version 8.x I guess?) Perhaps this-binding is going to stick around past that, or perhaps all the this binding is going away? I wasn't clear from what it says.

---

In any case, if we think about this issue as a request for a new feature in version 8, I'm imagining it would involve minor changes in two spots, the subscribe(..) function and the SafeSubscriber(..) constructor:

1. the subscribe(..) function needs to look in the "partial observer" object passed into it for something like a _next or noBind or skipBind present, and if so, pass an additional boolean to the SafeSubscriber(..) constructor:

    subscribe(
       observerOrNext?: Partial<Observer<T>> | ((value: T) => void) | null,
       error?: ((error: any) => void) | null,
       complete?: (() => void) | null
     ): Subscription {
   +    const skipBind = (observerOrNext.skipBind === true);
   -    const subscriber = isSubscriber(observerOrNext) ? observerOrNext : new SafeSubscriber(observerOrNext, error, complete);
   +    const subscriber = isSubscriber(observerOrNext) ? observerOrNext : new SafeSubscriber(observerOrNext, error, complete, skipBind);

2. A new parameter (called skipBind or whatever) in the SafeSubscriber(..) constructor signature, which defaults to false:

   subscribe(
       observerOrNext?: Partial<Observer<T>> | ((value: T) => void) | null,
       error?: ((error: any) => void) | null,
   -   complete?: (() => void) | null
   +   complete?: (() => void) | null,
   +   skipBind = false
     ): Subscription {

   ....and a single conditional in the SafeSubscriber constructor:

   - next = next?.bind(context);
   + next = skipBind ? next : next?.bind(context);

I may be missing other details, but I think that seems like it would essentially cover what I'm suggesting.

[kwonoj]

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.

[getify]

in my opinion this is very close to implementation detail

When I posted this issue I didn't know that the this-binding of subscriber next functions was a thing. It seemed like an internal-only implementation detail. After poking around on this topic, it's now clear that it's an intentional, user-facing feature.

First, what'll happen if we internally decide to not to use bind at all?

Since it's a real feature, it would be a disruptive breaking change for you to later remove this feature from Rx. Even if you did decide to make such a breaking change, you'd be deprecating it for a release or two and then later actually removing it. In that case, you'd also be deprecating and removing the skipBind or whatever it would be called.

Even if, hypothetically, you didn't deprecate and gracefully remove the this-binding feature, but just made a quick breaking change to remove it (for some outlandish reason)... the hanging skipBind in user code would then just be ignored and pointless, but not breaking... it's not like having introduced that flag would lock you into a behavior you didn't want to keep -- since you (hypothetically) removed this-binding, you no longer care if people want to opt-out of a now-non-existent behavior!

What I'm suggesting is intended to opt-OUT of a feature, not OPT-IN to some other feature. My point: since it's a user-facing "feature", it's not just an internal implementation detail.

I do not think we can skip this request is coming from your code's specific usecases

I think it's unfortunate that you're unwilling to shift mental contexts. Had I never mentioned anything about breaking functions and was only talking about the performance optimization opt-out, such a hang-up wouldn't be relevant to the discussion.

What is actual performance overhead we are currently seeing?

That's a perfectly good and fair question. I don't have a concrete answer to that, but I can tell you that bind(..) has historically had performance costs in various JS engines, so even if it's better now, it's not zero.

But it would indeed be interesting to benchmark what sort of performance improvement occurs if the bind(..) is skipped. I'm not nearly familiar enough with Rx to accurately do that with your library (testing all its various entry paths). But I could certainly construct a bare-bones JS benchmark for whether putting a pointless bind() call in costs anything, and moreover whether every call to the pointlessly-bound function is any noticeably slower than just directly calling the function. In fact, I'm sure others have done so on many occasions -- the research for bare/native JS probably already exists!

However, absent such research, it seems you're already jumping to the assumption/conclusion -- that it's negligible -- before we've looked into it.

Moreover, I think we should think of the performance implications from two angles:

1. The docs in multiple places suggest people do things like subscribe({ next: v => whatever(..), ... }). IOW, it's actually suggested that people pass => arrow functions in as these subscriber callbacks; certainly a lot of developers are thus doing so.

   That means NONE of them are benefitting from the this-binding that's happening. Whatever micro/milliseconds are being spent on the bind(..) for all of them, it's being wasted (and as we know, small things add up to big things when done a lot). Some of those folks might be happy to see a release-note in a future version that says they can simply add a boolean flag and get a minor speed up with no other code changes.

2. In the other direction, there's probably some folks out there who are passing their own this-aware functions in as subscriber callbacks. For them, the this-binding that Rx does may actually be an "anti-feature" in that their functions may rely on their own this and not want Rx messing with it. For those folks, they've previously had two options:

   • wrap their callbacks in a lambda (thereby wasting an extra call on the call-stack, AND wasting the bind(..))
   • do their own bind(..) on the function before they pass it in (thereby wasting the extra bind(..))

   In either case, they're presently "working around" Rx's behavior, which is costing a tiny performance hit to do so. I bet many of them would love a note in a future release that allowed them to pass a boolean flag and get rid of their work-arounds.

Perhaps it's at least worth discussing if such a minor performance-oriented feature might serve some folks well.

[kwonoj]

I think fundamental different point of view is what you consider as user-facing, non implementation detail is in my opinion it is implementation detail that a library can make necessary changes anytime. 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.

I think I have expressed my own opinion enough and do not think it's ok to repeat mine again. Issue will leave as opened for now in case if any other core member want to share their opinions.

[getify]

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.

[benlesh]

I see the issue and it's a minor fix.

That said, people shouldn't be patching known methods on functions. Haha.

[benlesh]

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

[benlesh]

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.

[getify]

@benlesh

Basically, @getify, you're saying "please don't rely on the behavior of Function.prototype.bind because someone could patch it"

That's not what I'm asking. I'm not asking you to change any of your assumptions about how people use your library, nor any default behaviors. Even if I don't use or care about those behaviors, I assume some people do, and that's fine.

I am however observing that even your documentation most commonly provides non-this-aware functions (as arrow functions), so I think there's a substantial portion of users, myself included, who are providing your library non-this-aware functions. For those us doing so, I was suggesting/requesting an additional affordance: to opt-out of your library doing the this binding. That's it, plain and simple.

If I provide you a non-this-aware function (like an arrow function), you calling bind(..) on it is totally pointless and wasteful (even a tiny bit of CPU). If I have a this-aware function that needs a different this than the one your library is assuming to bind(..) for me, then I have to do my own bind(..) calls, in which case your library calling bind(..) on an already bound function is, again, totally pointless and wasteful (even a tiny bit of CPU).

There must be a good chunk of Rx users in one of those two buckets... so on behalf of them, I was asking for the ability to opt-out of that path. How to opt-out is entirely subjective bikeshedding and I don't really much care what such an opt-out would look like.

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?

Speaking of red herrings, I definitely regret mentioning this fact in the subject/OP, as it's not at all the point anymore. As noted several times, now that I understand the situation better, I have shifted my attention away from "it's broken!" to "can I have an opt-out?". Perhaps I should have just closed this thread and opened a new fresh one without the prior baggage.

But again, the simple "use case" is: I am providing you a function which does not need or want your bind(..) call performed against it, and I'd like to tell you that you can and should skip doing so. It has nothing to do with whether such unnecessary binding would break or not. It's just about avoiding unnecessary operations.