Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

【RFC】Wechaty support middleware. #2282

Closed
Gcaufy opened this issue Oct 26, 2021 · 16 comments
Closed

【RFC】Wechaty support middleware. #2282

Gcaufy opened this issue Oct 26, 2021 · 16 comments

Comments

@Gcaufy
Copy link
Contributor

Gcaufy commented Oct 26, 2021

问题

Wechaty 已经开发过 plugin 机制,参考:

但 plugin 的粒度还是相对较粗,导致不同的 plugin 之间还是存在许多重复代码。

vote-out 插件为例,假设我们有个需求,需要写个 vote-up 来记录每天群内谁被点贊最多,此时就会发现 vote-out 的所有过滤代码都得原封不动的拷贝至 vote-up 中,然后再完成 vote-up 的真实逻辑。
所以根本原因是 plugin 中的逻辑无法被更小粒度的抽象和复用,

思考

很早之前也粗略的思考过这里的问题,但改造成本较高,且与历史API存在兼容性问题,但最近又思考了一下,发现还是可以解决的。
可以扩展一种针对事件维度的 middleware 机制(洋葱模型)进行更小粒度的抽象。

示例代码如下

在现有模式下,如果要完整类似功能,需要这么写。

// room a 需要 vote-out 功能
bot.on('message', async (message) => {

  const room = message.room()

  if (!room) return 
  if (message.type() !== type.Message.Text) return 
  if (!matchers.roomMatcher('room a')) return

  console.log('do vote-out action');
});
// room b 需要 vote-up 功能
bot.on('message', (msg) => {

  const room = message.room()

  if (!room) return 
  if (message.type() !== type.Message.Text) return 
  if (!matchers.roomMatcher('room a')) return

  console.log('do vote-up action');
});

设计事件维度中间件能力后,可以这样写。

// 抽象通过能力为中间件,由中间件控制事件响应
const filterMiddleWare = (options) {
  return async (message, next) => {

    const room = message.room()

    if (!room) return 
    if (message.type() !== option.type) return 
    if (!matchers.roomMatcher(option.room)) return

    await next();
  }
}

// 只有 room a 中会响应, vote-out 事件
bot.on('message', 
  filterMiddlerWare({ type: type.Message.Text, room: 'room a'}),
  (message) => console.log('do vote-out action')
);

// 只有 room b 中会响应, vote-up 事件
bot.on('message', 
  [ filterMiddlerWare({ type: type.Message.Text, room: 'room a'}) ],  // support middleware array.
  (message) => console.log('do vote-up action')
);

对于现有插件,我们也可以扩展 use 方法在执行前去装载中间件

// VoteOut 插件只对 room a 生效
bot.use(VoteOut({ /* options */ }), { message: [ filterMiddleWare({ room: 'room a'}) ] })

这样,就能无缝的兼容原来的 API 和 插件能力啦 。

未来插件也可以做得更轻,只专注于功能本身,而不用去写一堆的 if (xxxx) return 啦。
而同时,插件本身也可以通过拼接一堆 middleware 来完成一个大的功能插件。

变更

on (event: WechatyEventName, listener: (...args: any[]) => any): this
on (event: WechatyEventName, middlewares: WechatyMiddleWare | WechatyMiddleWare[], listener: (...args: any[]) => any): this


use(...plugins, option: WechatyPluginMiddleWare);

Related issues

@binsee
Copy link

binsee commented Oct 27, 2021

两处代码块中 vote-up 中对应的 room 应为 room b,看起来是复制时忘改了

@huan
Copy link
Member

huan commented Nov 4, 2021

Recently I'm reading the W3C EventTarget and Node.js EventTarget, which is the browser version of the EventEmitter, and they both have very similar (compatible) API interfaces.

What attracted me the most is, the event.preventDefault() feature of the EventTarget.

As we know the EventEmitter listeners can only control what happens inside itself, and it can not prevent the listener chain to continue.

But the EventTarget can call the event.preventDefault() to stop all the listeners after the current one.

I'm thinking about switching from EventEmitter to EventTarget because of this feature.

@Gcaufy What do you think?

@Gcaufy
Copy link
Contributor Author

Gcaufy commented Nov 5, 2021

We'd better not to stop events inside one event, because there is no way to reuse the event stopped by other events.

For example

// I need to write code `Only print apple in room-a`
bot.on('message',  (message) => {
  if (/* room is room-a */) {
    // continue;
  }
  // If it's not room-a, then stop all other events 
  event.preventDefault();
}

// This works prefect as what we expected.
bot.on('message', (message) => {
  console.log('apple');
});

// BUT, things changed, I need to print banana to all the other rooms, include room-a
bot.on('message', (message) => {
  console.log('banana');  // This won't work, because the other events was stopped, and no way to reuse it.
});

So, if we need to make it right, we need to move the 3rd event to the top, which means we need to be starkly aware of the event sequence.
We could barely to this if the code is quite complicated.
Especially, if we are using many plugins (which has event.preventDefault() inside), plugins interfere with each other, different use sequence getting different results.

So in this case, I think EventEmitter is better than EventTarget.

@huan
Copy link
Member

huan commented Nov 8, 2021

I have been thinking about the Middleware proposal for the past few days and I must say this is a brilliant design. Thank you very much @Gcaufy!

I think basically the whole middleware design is good, we definitely can move forward based on that.

Below is some of what I thought about this design:

  1. Can we reuse the wechaty.use() method to also support the middleware? My idea is: the middleware will have more than one (>1) function arguments (like: middleware(...args: any[], next: Function), but the plugin will only have one (=1) (which is plugin(wechaty: Wechaty)). Maybe we can use the Function arguments signature to differentiate them.
  2. In the design, I saw that each middleware needs to bind to one Wechaty event, and I understand that. For example, a middleware is dealing with message, and another is dealing with friendship. Can we encapsulate this event name inside the middleware, instead of specifying the name outside? For example, it would be more simple if we can rewrite the below code:
    - bot.use(VoteOut({ /* options */ }), { message: [ filterMiddleWare({ room: 'room a'}) ] })
    + bot.use(VoteOut({ /* options */ }), filterMessageMiddleWare({ room: 'room a'}))
  3. I feel each Wechaty event listener is a special middleware that has no next to call. If so, can we generalize the middleware definition for Wechaty listeners? What I think about is: can we simply treat all listeners as middleware?
  4. The on()/addListener() method: those methods are extended from the EventEmitter from Node.js, which I think we should keep their call signatures not changed as possible as we can. So in order to support middle, we might need a factory method, to compose the middleware and the listeners together, then we can use the returned function as a listener, to register with on() and addListener(). for example:
    bot.on('message', middlewareListener(
      filterMiddlerWare({ type: type.Message.Text, room: 'room a'}),
      (message) => console.log('do vote-out action'),
    ))
  5. The last one: do you think we need to add a context argument (it can be this) to our middleware? I think we should have some kind of context because it can deliver a message between middleware, and it is essential. We have some options, for example:
    1. this as context: the middleware function will have a this as its context (what should be its initial context schema?)
    2. add a context args as the first argument. (looks verbose?)
    3. other possibilities?
  6. The last one+1: WechatyImpl in wechaty@1.x has been hidden inside the wechaty/impls and does not recommend users to use it directly. The recommended way is to use WechatyBuilder. How do you think let's move the static set middleware method to the WechatyBuilder? For example:
    const builder = WechatyBuilder.new().middleware(...middlewares)
    const wechaty1 = builder.build()
    const wechaty2 = builder.build()
    // wechaty1 and wechaty2 has all added the middlewares

The above is what's in my head now, would love to hear any feedback, and let's push this RFC design move forward together.

@huan huan pinned this issue Nov 8, 2021
@echofool
Copy link

echofool commented Nov 8, 2021

作为 .NET 平台上的开发者,我建议参考 ASP.NET CORE middleware 的设计
从使用者的体验角度来说设计成如下风格,中间件采用洋葱模型

//添加全局 middleware
bot.messages
     .use(middleware1)
     .use(middleware2)
//添加指定筛选的 middleware
bot.messages
     .where(filter)
     .use(middleware3)
     .use(middleware4)

middleware 的效果类似于如下签名的函数

//其中 next 代表下一个管道,可以决定是否继续执行下一层
handler(context, next) :Promise

实际编写的时候可以如下面的效果:

bot.messages
     .use(async (context, next) -> {
        console.log("global middleware 1")
        await next(context)
     })
     .use(async (context, next) -> {
        console.log("global middleware 2")
        await next(context)
     })
     .where(message->message.room()=="a")
     .use(async (context, next) -> {
        console.log("middleware 3 with filter")
        await next(context)
     }).use(async (context, next) -> {
        console.log("middleware 4 with filter")
        await next(context)
     })

以上代码中的 where 类似于 ASP.NET CORE middleware 的设计 中的Branch the middleware pipeline例子里面的Map

@Gcaufy
Copy link
Contributor Author

Gcaufy commented Nov 8, 2021

作为 .NET 平台上的开发者,我建议参考 ASP.NET CORE middleware 的设计 从使用者的体验角度来说设计成如下风格,中间件采用洋葱模型

//添加全局 middleware
bot.messages
     .use(middleware1)
     .use(middleware2)
//添加指定筛选的 middleware
bot.messages
     .where(filter)
     .use(middleware3)
     .use(middleware4)

middleware 的效果类似于如下签名的函数

//其中 next 代表下一个管道,可以决定是否继续执行下一层
handler(context, next) :Promise

实际编写的时候可以如下面的效果:

bot.messages
     .use(async (context, next) -> {
        console.log("global middleware 1")
        await next(context)
     })
     .use(async (context, next) -> {
        console.log("global middleware 2")
        await next(context)
     })
     .where(message->message.room()=="a")
     .use(async (context, next) -> {
        console.log("middleware 3 with filter")
        await next(context)
     }).use(async (context, next) -> {
        console.log("middleware 4 with filter")
        await next(context)
     })

以上代码中的 where 类似于 ASP.NET CORE middleware 的设计 中的Branch the middleware pipeline例子里面的Map

这个是 middlewares 本来应该有的样子,完全赞同。
但是当前事件消息完全基于 EventEmitter 的,这样设计可能会完全重构,成本非常高。

@Gcaufy
Copy link
Contributor Author

Gcaufy commented Nov 8, 2021

  1. Can we reuse the wechaty.use() method to also support the middleware? My idea is: the middleware will have more than one (>1) function arguments (like: middleware(...args: any[], next: Function), but the plugin will only have one (=1) (which is plugin(wechaty: Wechaty)). Maybe we can use the Function arguments signature to differentiate them.

同意使用 use, 但是目前代码 plugin 也是支持多个参数的,所以可能不太好作区分,除非直接改掉,不向下兼容了。

  1. In the design, I saw that each middleware needs to bind to one Wechaty event, and I understand that. For example, a middleware is dealing with message, and another is dealing with friendship. Can we encapsulate this event name inside the middleware, instead of specifying the name outside? For example, it would be more simple if we can rewrite the below code:

意思是 middleware 同时返回 eventname 和 function 吗?最初我也这么想过,但考虑到限制死了 middleware 只能用过这个 event了, 所以还是放在外面声明了。比如说, 有个 middleware 在 message 和 friendship 上面都可以用。那该如何去复用呢?

  1. I feel each Wechaty event listener is a special middleware that has no next to call. If so, can we generalize the middleware definition for Wechaty listeners? What I think about is: can we simply treat all listeners as middleware?

同意,我也是这么想的。

  1. The on()/addListener() method: those methods are extended from the EventEmitter from Node.js, which I think we should keep their call signatures not changed as possible as we can. So in order to support middle, we might need a factory method, to compose the middleware and the listeners together, then we can use the returned function as a listener, to register with on() and addListener(). for example:

Good idea. 写的时间没考虑到这种方式。后面我再看看这种实现。

  1. The last one: do you think we need to add a context argument (it can be this) to our middleware? I think we should have some kind of context because it can deliver a message between middleware, and it is essential. We have some options

这个我也想过,把所有事件属性都放在 Context 下,但可能会出现不同事件只 Context 内容大不相同,而且对 listener 也存在向下不兼容的问题。

  1. The last one+1: WechatyImpl in wechaty@1.x has been hidden inside the wechaty/impls and does not recommend users to use it directly. The recommended way is to use WechatyBuilder. How do you think let's move the static set middleware method to the WechatyBuilder

同意。 代码里我也设计了 global 的 middlewares.

@wj-Mcat
Copy link
Contributor

wj-Mcat commented Nov 8, 2021

另外一个想法: Plugin can be a type of middelware.

初衷

你设计middleware的初衷就是:plugin 的粒度还是相对较粗,导致不同的 plugin 之间还是存在许多重复代码。

那我就想,我们只需要解决plugin的代码复用性的问题就行了,这个时候我想解决办法有很多种,其中的一种就是让插件之间存在一定的有向拓扑依赖关系,然后在wechaty启动的时候就编译成一个plugin tree,同一个tree level的plugins可以并行emit,不同level之间需要同步执行。

草图

middleware-structure

simple code

from typing import List, Any


class Plugin1(WechatyPlugin):
    def __init__(self):
        self.plugin_global_variables: Any = {}
        self.plugin_conversation_variables: Any = {}
    
    def do_something(self):
        pass


class Plugin2(WechatyPlugin):
    def __init__(self):
        self.plugin_global_variables: Any = {}
        self.plugin_conversation_variables: Any = {}

    def do_another_thing(self, data: Any):
        pass


class Plugin3(WechatyPlugin):
    pass


class Plugin4(WechatyPlugin):

    def dependencies(self) -> List[WechatyPlugin]:
        return [Plugin1, Plugin2]
    
    def customized_func(self):
        plugins = self.get_dependency_plugins()
        plugin1: Plugin1 = plugins[0]
        plugin2: Plugin2 = plugins[1]

        result = plugin1.do_something()
        # but plugin2 don't require plugin1
        plugin2.do_another_thing(result)

@huan
Copy link
Member

huan commented Nov 9, 2021

Can we reuse the wechaty.use() method to also support the middleware? My idea is: the middleware will have more than one (>1) function arguments (like: middleware(...args: any[], next: Function), but the plugin will only have one (=1) (which is plugin(wechaty: Wechaty)). Maybe we can use the Function arguments signature to differentiate them.

同意使用 use, 但是目前代码 plugin 也是支持多个参数的,所以可能不太好作区分,除非直接改掉,不向下兼容了。

After some time, I think there should be some way to know them at some point...

function use (fn: Plugin | Middleware) {
  if (fn.length === 1) { 
    fn(this) // <- this is plugin, expecting one arg: wechaty
  } else if (fn.length === 2) {
    fn(context, () => {}) // <- this is middleware
  } else {
    console.error('what fn I have got?')
  }
}

Please correct me if I have missed anything.

@huan
Copy link
Member

huan commented Nov 11, 2021

Should we support removing middleware/plugins after they have been installed?

  1. uninstall a middleware?
  2. uninstall a plugin?

Currently, once a plugin or middleware has been installed, there's no way to remove it.

So does the Express middlewares.

huan added a commit that referenced this issue Nov 11, 2021
* restructure mixins

* export use singlar instead of plural

* export use singlar instead of plural

* 1.3.5

* comment

* 1.3.6

* update puppet

* 1.3.7

* update puppet

* update puppet

* 1.3.8

* add unitt tests for mods

* fix circle dependency problem

* 1.3.9

* add valid method for WechatyBuilder

* rename to Accepter

* 1.3.10

* clean unit test for plugin (#2282 (comment))

* 1.3.11

* fix smoke testing

* v1.5

* 1.5.1
@Gcaufy
Copy link
Contributor Author

Gcaufy commented Nov 12, 2021

function use (fn: Plugin | Middleware) {
  if (fn.length === 1) { 
    fn(this) // <- this is plugin, expecting one arg: wechaty
  } else if (fn.length === 2) {
    fn(context, () => {}) // <- this is middleware
  } else {
    console.error('what fn I have got?')
  }
}

Please correct me if I have missed anything.

It make sense, but

  1. Normally plugin have 1 argument, and middlewares have more, but If user use js, he can pass less arguments or more.
  2. They both use use, will it make user confused, the arguments logic is only in the code, how does user recongnize that.

How about we use install for plugins, and use use for middleware

@Gcaufy
Copy link
Contributor Author

Gcaufy commented Nov 12, 2021

Are we sure to use Context instead of use expanded arguments? If yes, then what's in it? simply put everything in it?.

I still prefer to use expanded arguments.

  1. Context is using for request, because every request would have Req and Resp etc.
    But in EventEmitter, different event have diffrent paramter.
    In this case, it would Be MessgeContext, FriendShipContext, etc.

  2. As you said, the event listener also can be a middlerware. So that means the listener should have the same arguments with middleware.

@Gcaufy
Copy link
Contributor Author

Gcaufy commented Nov 21, 2021

@huan Please check the comments so that I can continue :0)

@huan
Copy link
Member

huan commented Nov 21, 2021

Dear @Gcaufy ,

Thank you very much for pushing this RFC forward, it's a great design, so please go ahead with the PR and make the CI green, then we start discussing based on the draft code, and run some example code based on your new code and test the user experiences for developing.

This week I'm mainly working on the below PR, will back to you after I have finished it:

Below are some of my options to our previous question:

Typing change of on() & use()

I believe your original designs are in the right direction. Please update when necessary, and let's follow this design for our middleware draft design.

on (event: WechatyEventName, listener: (...args: any[]) => any): this
on (event: WechatyEventName, middlewares: WechatyMiddleWare | WechatyMiddleWare[], listener: (...args: any[]) => any): this

use(...plugins, option: WechatyPluginMiddleWare);

Question: how to deal with room-invite (and some similar to it) event?

The room-invite event has more than one argument:

type RoomEventListenerInvite = (this: RoomInterface, inviter: ContactInterface, invitation: RoomInvitationInterface) => void | Promise<void>
type RoomEventListenerJoin = (this: RoomInterface, inviteeList: ContactInterface[], inviter: ContactInterface, date?: Date) => void | Promise<void>
type RoomEventListenerLeave = (this: RoomInterface, leaverList: ContactInterface[], remover?: ContactInterface, date?: Date) => void | Promise<void>
type RoomEventListenerMessage = (this: RoomInterface, message: MessageInterface, date?: Date) => void | Promise<void>
type RoomEventListenerTopic = (this: RoomInterface, topic: string, oldTopic: string, changer: ContactInterface, date?: Date) => void | Promise<void>

How do we define a middleware for it?

One middleware interface for all event types?

In the design, I saw that each middleware needs to bind to one Wechaty event, and I understand that. For example, a middleware is dealing with messages, and another is dealing with friendship. Can we encapsulate this event name inside the middleware, instead of specifying the name outside? For example, it would be more simple if we can rewrite the below code:
意思是 middleware 同时返回 eventname 和 function 吗?最初我也这么想过,但考虑到限制死了 middleware 只能用过这个 event了, 所以还是放在外面声明了。比如说, 有个 middleware 在 message 和 friendship 上面都可以用。那该如何去复用呢?

Could you please provide an example middle that "在 message 和 friendship 上面都可以用"?

After I think it more, maybe we can also consider that one middleware only deals with one event type?

The context

Context is used for requests because every request would have Req and Resp etc.

I'm always thinking about that, the interaction between the chatbot and the user is very similar to the Web server and the user., because as you said: "every request would have Req and Resp", the conversation between the chatbot and user have Turn (a concept from BotFramework) for a question/answer (request/response), and the bot needs to store information between turns, which needs a context to store those pieces of information, and BotFramework has a TurnContext for that.

However, as our listener functions will have a this: Wechaty, I believe we can use the wechaty.memory to store that information. If we use wechaty.memory as the context of the turn, then we do not need to do anything additionally.

wechaty.memory is a MemoryCard instance with a ES6 Map like API in Async.

@huan huan unpinned this issue Apr 14, 2022
@leochen-g
Copy link
Contributor

@Gcaufy Hello, are you still following this issue? This problem is also bothering me.

Copy link

dosubot bot commented Nov 19, 2023

Hi, @Gcaufy! I'm Dosu, and I'm here to help the wechaty team manage their backlog. I wanted to let you know that we are marking this issue as stale.

From what I understand, you proposed adding middleware support to Wechaty to reduce code duplication and improve code reusability between plugins. There have been discussions about the design and implementation details, including the use of EventTarget, the structure of middleware, and the use of Context. The author and maintainers are actively discussing and refining the proposal.

Before we close this issue, we wanted to check with you if it is still relevant to the latest version of the wechaty repository. If it is, please let us know by commenting on the issue. Otherwise, feel free to close the issue yourself or it will be automatically closed in 7 days.

Thank you for your contribution and understanding. Let us know if you have any further questions or concerns.

@dosubot dosubot bot added the stale Issue has not had recent activity or appears to be solved. Stale issues will be automatically closed label Nov 19, 2023
@dosubot dosubot bot closed this as not planned Won't fix, can't repro, duplicate, stale Nov 26, 2023
@dosubot dosubot bot removed the stale Issue has not had recent activity or appears to be solved. Stale issues will be automatically closed label Nov 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants