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

feat: added wechaty plugin #1946

Merged
merged 14 commits into from
May 1, 2020
Merged

feat: added wechaty plugin #1946

merged 14 commits into from
May 1, 2020

Conversation

Gcaufy
Copy link
Contributor

@Gcaufy Gcaufy commented Apr 18, 2020

Wechaty.use

[ ] Bug Fix
[x] Feature
[ ] Other (Refactoring, Added tests, Documentation, ...)

Checklist

  • Commit Messages follow the Conventional Commits pattern
    • A feature commit message is prefixed "feat:"
    • A bugfix commit message is prefixed "fix:"
  • Tests for the changes have been added

Description

#1939

We can discuss it here.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

@Gcaufy
Copy link
Contributor Author

Gcaufy commented Apr 18, 2020

As the discussion here: #1939

I made few changes of the demo code. Just want to make sure you are agree with it.
So I can continue on the rest

1. I would like to call it Plugin instead of MiddleWare

In my opinion, middleware is alway dealing with the input/output. Middleware A deal with it, and then decide to handover to Middleware B. But we are not doing that. We just installed it, and then it do works for you. It's more like a Plugin.

I think a MiddleWare is something like this:

Wechaty.use({ message: function (message: string, next: (message: string) => void) {
  if (message.indexOf('hi') > -1) {
     next();
  }
})

So that's the reason I change the name to Plugin. and implement it like this.

2. I would prefer to use a abstract class as a Plugin instead of a plugin function.

// 1. Plugin function
const MyPlugin = function (options: any) {
  return function(this: Wechaty) {
    this.on('join-room', () => console.log('joined'));
  }
}
Wechaty.use(MyPlugin({a: 1}));
// If I install the plugin twice, what will it do? do work twice? it will make user confused unless they check the source code.

// 2. Plugin Class
Class MyPlugin {
  constructor(options) { this.options = options }
  install(bot: Wechaty) {
    bot.on('join-room', () => console.log('joined'));
  }
}
Wechaty.use(new MyPlugin({a: 1}));
// Install the same instance will not do work twice. easier for user to understand

Basically it does the same thing. but more readable, and easier to understand.

3. I would prefer the use function in Class instead of Instance.

Which is:

Wechaty.use(new MyPlugin());

but I made it both support the Class and the instance. also support the Array

Wechaty.use(new CommonPlugin());

const bot = new Wechaty();
bot.use([ new MyOwnPlugin(), new MyAnotherPlugin() ]);

@huan
Copy link
Member

huan commented Apr 19, 2020

Hi @Gcaufy ,

Thank you for the design of our powerful Middleware/Plugin system for Wechaty!

I have read your questions and also the source code in the PR, I believe we are in the right way to archive our goal.

Here's my reply for your 3 questions:

  1. Call it Plugin instead of MiddleWare

I have no preference between those two names because I feel they are all fit our design.

I agree with you that the middleware mostly they are chaining for doing something together, but in our system, it does not. I like the name of "Middleware" coz it sounds more fashion.

And the name of "Plugin" is more classic, simple, and clean, so they are all good.

Please feel free to make your decision about selecting the name, and I'll support your final choice!

  1. Plugin Interface: an abstract class, or a plugin function?

My opinion for the Interface is not to use a Class because it looks heavy.

I'd like to purpose the following design, to archive the same goals:

export type WechatyPlugin = (...options: any[]) => (wechaty: Wechaty) => void

const kickPlugin: WechatyPlugin = (managedRoomTopic: string) => wechaty => {
  wechaty.on('message', message => {
    ...
  })
}

wechaty.use(kickPlugin('My Room Topic'))

As my personal feeling, the above Functional design will be shorter and clearer than the Class design from yours.

So I'd like to purpose that our Plugin interface would be better to be defined as a Functional Interface.

  1. to define use() as a Class static method instead of the Instance method

Could you please explain it with more examples about why you want to design like that?

I feel it will be more clear and simple if we only provide a use() method for the Wechaty instance so that every instance is separated by default.


I'll also add some comments to the PR code, and please feel free to let me know what you think.

Thank you for your design and questions again, and I hope you have a great weekend!

src/wechaty.ts Outdated Show resolved Hide resolved
src/wechaty.ts Outdated Show resolved Hide resolved
src/wechaty.ts Show resolved Hide resolved
src/wechaty.ts Outdated Show resolved Hide resolved
src/wechaty.ts Show resolved Hide resolved
src/wechaty.ts Outdated Show resolved Hide resolved
src/wechaty.ts Show resolved Hide resolved
src/wechaty.ts Outdated Show resolved Hide resolved
@Gcaufy
Copy link
Contributor Author

Gcaufy commented Apr 20, 2020

Here are the conclusions, just want to make sure I'm not missing anything.

  1. Use the name 'Plugin' (keep it)
  2. Use both Wehcaty.use and wechaty.use (keep it)
  3. Remove the installed plugins property. (need changes)
  4. Change plugin to Functional style. (need changes)

btw. I probably need to work this in this weekend.

src/wechaty.ts Outdated
abstract installed?: boolean

export interface WechatyPlugin {
(this: Wechaty): void;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about let's use an explicit args for passing the wechaty instead of using this?

I'll write the reason in detail in the comments outside.

huan
huan previously approved these changes Apr 25, 2020
Copy link
Member

@huan huan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@huan
Copy link
Member

huan commented Apr 25, 2020

Everything looks good, thank you very much @Gcaufy !

I have one concern about the this for our Plugin function, I have left my comment as suggesting to use an explicit wechaty arg instead of the this, the reason is as the following:


In my experience in the past years, there are lots of developers who do not understand what this is in the JS function.

They will be very confused about the this and want to know how to use it, like the following screenshot, which was posted in the past week in our developers' home WeChat room:

image

image

I personally like to use this, and this is the current design of the event listener callback functions in Wechaty.

However, according to what the above screenshot shows out, I'd like to suggest that we use a wechaty arg to archive that, to make it easier to understand for all developers.

this.on('message', () => (result += 'FROM_MY_PLUGIN:'))
}
}
Wechaty.use(myGlobalPlugin())
Copy link
Member

@huan huan Apr 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

class WechatyTest extends Wechaty {}
WechatyTest.use(myGlobalPlugin())

Change this part to the above code would be better because it will not modify the global Wechaty class.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agree

src/wechaty.ts Outdated
@@ -152,6 +156,8 @@ export class Wechaty extends EventEmitter implements Sayable {
*/
private static globalInstance: Wechaty

private static globalPlugins: WechatyPlugin[] = []
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To make the variable name style to be consistent, I'd like to suggest to rename the globalPlugins to globalPluginList.

@Gcaufy
Copy link
Contributor Author

Gcaufy commented Apr 26, 2020

Everything looks good, thank you very much @Gcaufy !

I have one concern about the this for our Plugin function, I have left my comment as suggesting to use an explicit wechaty arg instead of the this, the reason is as the following:

In my experience in the past years, there are lots of developers who do not understand what this is in the JS function.

I don't like this either. One of the reason to use Class style is to avoid using this.

@huan
Copy link
Member

huan commented Apr 28, 2020

kindly ping @Gcaufy

I believe that after we change the this to (wechaty: Wechaty) and a few variable names, this PR is ready for merge.

I cannot be waiting to use this great new feature!

@Gcaufy
Copy link
Contributor Author

Gcaufy commented May 1, 2020

Changes:

  1. Renamed: globalPlugins -> globalPluginList
  2. Fix this in pluign
  3. Test case updated. use extends Wechaty
  4. A bug fix: when use extends, Wechaty.globalPluginList is wrong.

@huan huan merged commit 2f7b641 into wechaty:master May 1, 2020
@huan
Copy link
Member

huan commented May 1, 2020

Thanks to this great Plugin system, it will greatly improve the Wechaty ecosystem by letting everyone writing a powerful plugin to archive their needs with ease!

This feature will be available from Wechaty version 0.39.19

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants