Cannot remove a TextListener

[madr1c]

Bug Report

Expected Behavior

I wanted to use removeTextListener in order to remove one of my registered text listeners.

Actual Behavior

It doesn't remove the listener and returned null instead.

Steps to reproduce the Behavior

const originalRegex = new RegExp(/test/);
const similarRegex= new RegExp(/test/);

// Add the listener
bot.onText(originalRegex, (msg, match) => {
         const chatId = msg.chat.id;

         bot.sendMessage(chatId, 'Regex matched');
});

// Attempt to remove it
bot.removeTextListener(similarRegex)

Explaination

Currently your code for removing a listener looks like this:

removeTextListener(regexp) {
    const index = this._textRegexpCallbacks.findIndex((textListener) => {
      return textListener.regexp === regexp;
    });
    if (index === -1) {
      return null;
    }
    return this._textRegexpCallbacks.splice(index, 1)[0];
  }

This particular comparison (textListener.regexp === regexp) does not work with regular expressions. You should try to compare all relevant regex properties.

const compareRegex = (x, y) => 
            (x instanceof RegExp) && (y instanceof RegExp) && 
            (x.ignoreCase === y.ignoreCase) && (x.multiline === y.multiline) &&
            (x.source === y.source) && (x.global === y.global);

This would not only fix your bug it will also work in cases where a comparison between the stringified regexes won't. For example:

const originalRegex = new RegExp(/test/mg);
const similarRegex= new RegExp(/test/gm);

originalRegex.toString() === similarRegex.toString() // false
compareRegex(originalRegex, similarRegex) // true

[david1602]

I have that exact issue with my bot. It dynamically accepts new listeners and should be able to remove them, too.

Here is my dynamic adding of handlers:

https://github.com/david1602/1337/blob/master/utils.js#L108-L123

And here's how I try to delete them:

https://github.com/david1602/1337/blob/master/commands/delResponse.js#L17-L31

And that's because I need the initial reference. I added that because it just wouldn't match otherwise, as you can see here:

https://github.com/david1602/1337/commit/64cf8a1767f0ddbd4958b7ad5b6bb305d55ed266

I have also attempted adding gi to the new RegExp(response.regex), but that didn't make a difference because it expects the initial reference. As @madr1c mentioned, this will not protect it against a ig / gi mixup, but since I do not allow modifiers in the dynamic creation and create them myself this works as a temporary workaround.

[GochoMugo]

Consider this:

A user uses "similar" (as in their matching) regexps to register listeners. How will they be able to remove only one of the listeners, and not all? For example,

const regexp1 = /\/start/;
bot.onText(regexp1, function(msg, match) {
    // ... Am doing something e.g. tracking how many starts I receive, etc.
    // ... I do not handle the command. Am just observing here.
});

const regexp2 = /\/start/;
bot.onText(regexp2, function(msg, match) {
    // ... Am handling the command now!
});

// I want to remove only the first listener (and NOT the second one)
bot.removeTextListener(regexp1);

Currently, this will work. But should be compare the regexp's internal state, rather than the references, it ~is~ will basically be impossible to remove one of the listeners without removing the other(s).

[madr1c]

You are wrong.

Since the removeTextListener function uses findIndex to find the regexp it will just find and remove the first match. The comparator function used inside findIndex won't have any influence on that behaviour.

[GochoMugo]

What if I want to remove the second one? Consider removing only the second one without removing the first one? (that makes more sense...)

[madr1c]

That would be a completely new feature and has nothing to do with this bug in my opinion.

You should maybe open a new issue for your feature request.

[GochoMugo]

What I mean is if we compare internal states as demonstrated above, we could not tell which exact listener to remove since there will be a name/index collision in such a collection. Since we are currently matching the 1^st match (and that only), it would be impossible to remove a listener that happens to be at the 2^nd, 3^rd, n^th match; it would remove the 1^st match.

[madr1c]

Makes sense but what I mean is that we are currently experiencing a bug which causes the removeTextListener to not work at all. It would already help a lot to fix the comparison which I described in the first place.

From what I can see the removeTextListener function was never meant to organise the regexes in such a complex way. In order to fulfil your requirement the developer would have to rebuild the whole text listener administration.

We really need that fix. The new text listener administration would be a major change which needs time we don't have.

That is why I think you should open a new issue for your feature request.

[madr1c]

I would prefer avoiding a fork. If the collaborators are too busy could someone please consider granting me the privilege which allows me to push branches? I could open a PR then in order to speed this up a bit.

[GochoMugo]

I am trying to address your issue. My point is that the text listeners were designed to be identified using the reference of the corresponding RegExp object. Using your comparison method, would cause index collisions i.e. two or more listeners identified by the same RegExp object. We do not want such a situation where we are not sure which listener was to be removed. Imagine the weird bugs such a change would cause for someone who has used "similar" RegExp objects. Otherwise, suggest how we would handle such scenarios.

[madr1c]

I see your point. Regardless of that the current state is unacceptable because its not possible for stateless applications to keep the original object reference for the comparison.

What do you think of using a UNIX timestamp as our identifying attribute? We could easily store it in our database and it would have the advantage that it also indicates when the text listener was registered.

[madr1c]

You could do something like this for adding:

  /**
   * Register a RegExp to test against an incomming text message.
   * @param  {RegExp}   regexp       RegExp to be executed with `exec`.
   * @param  {Function} callback     Callback will be called with 2 parameters,
   *  the `msg` and the result of executing `regexp.exec` on message text.
   * @param  {mixed} [options] key             Key to identify the listener later.
   * @return  {mixed} The key under which the listener was registered
   */
  onText(regexp, callback, key = Date.now()) {
    this._textRegexpCallbacks.push({ regexp, callback, key });

    return key;
  }

And this for deleting:

  /**
   * Remove a listener registered with `onText()`.
   * @param  {RegExp} regexp RegExp used previously in `onText()`
   * @param  {mixed} [options] key  Key to identify the listener to be removed
   * @return {Object} deletedListener The removed reply listener if
   *   found. This object has `regexp` and `callback`
   *   properties. If not found, returns `null`.
   */
  removeTextListener(regexp, key) {
    const comparator = regexp ? 'regexp' : 'key'; 
    const index = this._textRegexpCallbacks.findIndex(textListener => {
      return textListener[comparator] === regexp ? regexp : key;
    });
    if (index === -1 || ('key' === comparator && !key) ) {
      return null;
    }
    return this._textRegexpCallbacks.splice(index, 1)[0];
  }

This wouldn't touch the current api and it would provide an option for the user to work with custom keys in order to identify the listeners. Haven't checked the whole application for possible side effects yet.

But does this look like something you could live with or do you have something completely different in mind?

[david1602]

@GochoMugo if we had a stateless application, how would you suggest we currently access these regex references? In the source I linked I used the internal API to find the reference I needed. However, internal API is never designed to be used outside. What am I missing out on?

[GochoMugo]

To be honest, I have not been in support of the onText() feature. It feels quite primitive. It feels incomplete. If we hope to solve this issue (and related ones to follow), we should work on improving these set of features i.e. listeners registered with RegExps.

But in the mean time, such a complex use case would need building your solution using the text event, rather than TelegramBot#onText(). The former gives you more control on how you want to handle your RegExps. The latter is for simple use cases.

And that is why I believe the TelegramBot#onText() is rather incomplete. The issue reported here is not a bug. We should classify this as a feature request. That way, we will focus on improving this feature.

What do you think? @madr1c @david1602

[david1602]

@GochoMugo do you think there are realistic scenarios where people register two different onText handlers instead of just doing it properly the first time? I think it's a question of definition whether someone should be able to register the same regex twice. In my opinion he shouldn't, because what happens if someone takes a regex like const regex = /a/g; and registers two different onText handlers with it? The current solution wouldn't cover this either and only delete the first one.

I think what @madr1c suggested is pretty okay. What bothers me personally is that I have to use the internal object of the bot to find the original reference to the regex, if I could just call a function which would return the regexes that would be an improvement. but not quite the solution.

What do you mean by the text event? Wouldn't .on('text') do the same as onText? Or what exactly do you mean by the text event?

After extremely much blabbering, there is the reasonable question: do you really want to support someone using the same regex (whether it be the same or a different instance), or would you want to register each regex only once, or at least risk deleting two if you add the same one twice? I mean if I used the same reference to register two onText handlers with the same object reference, how would I delete the second but not the first one?

[GochoMugo]

It is not up to us to decide whether a user may ever want to do such a thing. My major issue is correctness. If a user added two different listeners with the same reference, then they can not be able to remove them in a civilized manner. It will need some hacking around. But using the same reference can be viewed as the programmer's fault and not that of the library.

But the comparison proposed above changes how regexes are found internally (for everyone!). It might break someone's code as a regex that was previously unmatched is now matched. That is unacceptable!

Your use of the internal API depicts my point that this feature is incomplete. Using the text event gives you more control over how YOU handle the regexes. If your use case is too complex for the library to handle, you will have to use the text event for now.

Let us focus on improving this feature. I am not accepting the fix as suggested above. Again, to say that a user will never use "similar" regexes, is to acknowledge that we know exactly which kind of regexes they use. An impossibility...

So, how can this feature be improved to cater for your use cases?

[david1602]

In order to improve this, the bot should somehow allow to show all the registered text listeners to be able to properly remove them.

So either there needs to be an API to fetch all listeners (which could spit out a copy of the internal object for all I know) or/and a key could be added to each regex, as @madr1c suggested. This would also fix the issue I addressed, where you reference the same regex with two different handlers.

An automatically generated key could be added to the internals of each registration (which could just be a counting integer, or maybe be able to delete regexes by array index) and then you could delete by either throwing a regex reference in there (which would delete the first one) or you could use the index, which would allow for specific deletion of a certain one. You could fetch this with an instanceof RegEx. This would be both backwards compatible and solve the issue I think.

In any case, for now we're planning to fork this and make 2 separate PRs (I also have my own issue in this project, which is #354) to solve this for our own project for now. We can also link those here then and you can decide whether it makes sense or not and could be reused here.

Alternatively, if you want, you could add us as contributors to this project and we could directly open them here as well, and just discuss in the PR what we should and should not do.