removing handlers

[gabewillen]

Thank you for creating this library! I have a suggestion to enhance its functionality by allowing handlers to be removed.

The reason for this proposal is to address issues encountered when using handlers in a persistent manner within durable objects. Currently, it seems there's no way to remove listeners, which necessitates reinstantiating the client. This can lead to memory leaks and corrupted handlers.

Here are a few potential solutions, each with its own advantages and disadvantages:

1. Change handler arrays to protected:

   • Remove the private enforcement on the SlackApp handler arrays and change them to protected.
   • This would enable users to implement the removal of listeners at their discretion.
   • It wouldn’t affect the library's runtime and would maintain backward compatibility with existing users.

2. Use WeakRefs in the handler arrays:

   • Implement WeakRefs inside the arrays and validate the references before calling them.
   • This approach allows the garbage collector to manage memory efficiently, preventing leaks.
   • Although this requires a significant amount of code changes, it maintains backward compatibility and keeps the private attributes intact.

3. Add methods to facilitate handler removal:

   • Introduce new methods in the library specifically for removing handlers.
   • While this would require additional code changes and testing, it would also increase the library’s size.

By implementing one of these solutions, the library can better handle persistent use cases, preventing memory issues and enhancing overall stability.

[seratch]

Hi @gabewillen, thanks for the suggestion! I still don't fully understand why you think this is beneficial. Let me ask a few questions:

when using handlers in a persistent manner within durable objects

What is an example of a platform or scenario for this? More specifically, I'd like to understand what "durable objects" are.

which necessitates reinstantiating the client

What do you mean by "client" in this context?

It's indeed feasible to enable developers to access the array, but specifying the handler to remove could be complicated as they don't have easy-to-access IDs. Can you share a pseudocode demonstrating how your desired use case looks?

[gabewillen]

Thank you for your prompt response. Our situation is quite unique. Ideally, we would utilize a web application, but due to time constraints and our frontend team being occupied, we opted to develop a Slack app instead. In retrospect, I'm not entirely convinced it was the quicker option 😅.

when using handlers in a persistent manner within durable objects

What is an example of a platform or scenario for this? More specifically, I'd like to understand what "durable objects" are.

Durable objects refer to a feature provided by Cloudflare that offers persistent, transactional storage. You can find more detailed information here: Cloudflare Durable Objects.

We created an evaluation app for RLHF, which is used by our team members to grade responses directly in Slack. Although there are numerous other methods we could have employed to achieve similar functionality, the durable objects have been adequate given our tight deadlines and the typical startup urgency for rapid development.

which necessitates reinstantiating the client

What do you mean by "client" in this context?

When I mention "reinstantiating the client," I am referring to creating a SlackApp. One particular case involves our middleware, which uses your libraries internal authentication but also connects the Slack user to our internal database. With the use of durable objects, this connection process only needs to happen once. Although we could exit the middleware handler after the user lookup with a simple if-statement, the function would still be invoked, thereby consuming a small amount of computing resources unnecessarily. Ideally, we would remove this listener after the user is identified. Another example is the "app_home_opened" callback, which is used to initially load the view when a user opens the home tab. After this initial interaction, the handler is no longer needed due to subsequent user interactions.

It's indeed feasible to enable developers to access the array, but specifying the handler to remove could be complicated as they don't have easy-to-access IDs. Can you share a pseudocode demonstrating how your desired use case looks?

Given our specific requirements and the numerous arrays that store these handlers, I believe altering the API surface might not be the most practical solution. That's why I proposed simply changing the access level of these properties to protected. This adjustment should sufficiently meet our needs and anyone else needing a persistent instance of the app, without necessitating significant changes to the API structure.

[seratch]

Thanks for sharing the details. For your needs, I would suggest adding a simple if/else statement to the SlackApp initialization process as shown below:

export default {
  async fetch(
    request: Request,
    env: SlackEdgeAppEnv,
    ctx: ExecutionContext
  ): Promise<Response> {
    const app = new SlackApp({ env });
    if (!areDurableObjectsReady()) {
      app.use(durableObjectRelatedMiddleware);
    }
    return await app.run(request, ctx);
  },
};

When it comes to the "app_home_opened" event handler, the handler is still necessary even after a user gets their view. Every time a user accesses the view, your app is responsible for handling the event. If you don't want to refresh the view once it's available, you can simply do nothing in response to the event delivery.

Either way, I'd like to avoid exposing the internal arrays for this reason. The types/names of the arrays could be changed in the future (due to refactoring/Slack platform's breaking changes). It'd be appreciated if you could understand this.

I hope this makes sense to you, and the above suggestion is helpful. Is there anything else to discuss here?

[seratch]

Forgot to mention this: If I understand correctly, an edge function is stateless in general, though there may be runtime initialization cache. Thus, initializing SlackApp for each request should be inevitable.

[gabewillen]

Thanks for sharing the details. For your needs, I would suggest adding a simple if/else statement to the SlackApp initialization process as shown below:

export default {
  async fetch(
    request: Request,
    env: SlackEdgeAppEnv,
    ctx: ExecutionContext
  ): Promise<Response> {
    const app = new SlackApp({ env });
    if (!areDurableObjectsReady()) {
      app.use(durableObjectRelatedMiddleware);
    }
    return await app.run(request, ctx);
  },
};

When it comes to the "app_home_opened" event handler, the handler is still necessary even after a user gets their view. Every time a user accesses the view, your app is responsible for handling the event. If you don't want to refresh the view once it's available, you can simply do nothing in response to the event delivery.

Either way, I'd like to avoid exposing the internal arrays for this reason. The types/names of the arrays could be changed in the future (due to refactoring/Slack platform's breaking changes). It'd be appreciated if you could understand this.

I hope this makes sense to you, and the above suggestion is helpful. Is there anything else to discuss here?

Absolutely thanks again for hearing me out.

Forgot to mention this: If I understand correctly, an edge function is stateless in general, though there may be runtime initialization cache. Thus, initializing SlackApp for each request should be inevitable.

While this is true for workers and serverless in general. Durable objects are stateful and thus don't succumb to the same initialization processes. In essence you can create a durable id from a users slack id and they have their own globally available stateful instance to which every request can be routed too.

[seratch]

Durable objects are stateful and thus don't succumb to the same initialization processes. In essence you can create a durable id from a users slack id and they have their own globally available stateful instance to which every request can be routed too.

Yes, the DO data is indeed stateful, but I don't understand the connection between the data and framework code change. Usually, developers look up the data within handler code. Thus, registering handlers is still necessary, and you can dispatch an event with consideration of the DA data within the functions.

The whole handlers can be omitted for some scenarios. As I suggested above, the proper way to do so is to have if/else statements during the SlackApp initialization. This means you have to have your own request payload parser to determine whether you should register a handler. This requires extra work on your side. I'm not sure if this is what you would like to have.

If you want to continue this discussion further, could you share a pseudo code demonstrating your ideal goal? Otherwise, is it okay to close this issue?

[seratch]

Also, I believe that registering handlers itself never costs a lot in terms of resource consumption (in comparison with others like DA data lookup etc.). But if such a small cost is critical for your use case, please go ahead with if/else statements during the initialization. You should be able to optimize the cost by doing so.