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] We need a better error system #159

Open
windmemory opened this issue May 13, 2019 · 6 comments
Open

[RFC] We need a better error system #159

windmemory opened this issue May 13, 2019 · 6 comments
Labels
enhancement New feature or request

Comments

@windmemory
Copy link
Member

Is your feature request related to a problem? Please describe.
Recently we observed more and more limitation pushed out by WeChat. In some cases, connection to WeChat server got kicked out. It would be better that we emit these cases as errors so developers could handle them properly.

Currently all the error messages emitted is an Error instance, only one message on it, so it is really hard to set a solid error type for the error. So here is this issue, I would like to bring this issue up and talk about the design for WechatyError or something else :P

Describe the solution you'd like
We could have a parent error type as WechatyError, then PuppetError extends the parent, then each puppet has its own error class.

How about this design:

class WechatyError extends Error {
  public readonly type: WechatyErrorType
  public readonly message: string
  constructor (
    type: WechatyErrorType,
    message: string,
  ) {
    super()
    this.type = type
    this.message = message
  }
}

Or we could make this parent error as an abstract class, then all the other errors extends this one.

Describe alternatives you've considered
Haven't considered any alternatives...

Additional context
I've implemented one temporary solution in wechaty-puppet-padpro, but that's just a test, you could check it here:
wechaty/wechaty-puppet-padpro@47d8de1

Here is a small piece of code that how I use it:

bot
.on('error', error => {
  const errorMessage = error.message
  const subMessages = errorMessage.split(' ')
  if (subMessages[0] === 'PADPRO_ERROR' && subMessages[1] === 'LOGIN') {
    console.log(`-----------------------------------------------------
    AutoLogin Error:
    type: ${subMessages[2]}
    message: ${subMessages[3]}
    ------------------------------------------------------`)
  }
})
.start()

To be honest, this is ugly, but just treat this as 【抛砖引玉】(don't know how to translate this :P)

Let's make wechaty better and better.

[enhancement]

@huan
Copy link
Member

huan commented May 13, 2019

Thanks for the RFC, it's a very good proposal.

Could you be able to provide 3 of the best examples that you could find in other open source projects, and points out which part do you think they are the best?

I believe we need more 砖 to study first, then we will be able to discuss deeper based on them.

@windmemory
Copy link
Member Author

I could try to find some good examples. And will post it in the issue.

In the mean time, how about tensorflow, what kind of error classes do they have? Since you've been working on that project for a period of time, could you please share a little about that?

@huan
Copy link
Member

huan commented May 24, 2020

After thinking this RFC for the past 12 months, I believe that we can move forward to continue discussing how to implement it because we will have some huge improvements for the Wechaty recently after we landing the Multi-language Wechaty.

@windmemory Would you like to share some more thoughts from your experiences in the past year? I believe it would be a great help for us to understand what is the best design of our Error system.

@windmemory
Copy link
Member Author

From the previous experience, I think there are two key properties needs to be reported when an error happened. They are:

  • error type (used to distinguish different errors)
  • whether is recoverable (so we can decide whether we should retry the operation)

Error type

I think we can achieve this feature in two ways

  • add a property type to the error class, we can distinguish the error by the type
  • extend the base error class with different class, we can distinguish the error by instanceof function

Recoverable

This can be solved in similar ways as above.

  • add a property recoverable to the error class
  • extend the base error class, create RecoverableError and NonRecoverableError, then all the other error needs to extends these two classes.

Other topic needs to be considered and discussed

  • where should we declare and implement these errors?
  • how a developer extends the errors we have currently
  • how to transfer these class to different languages?
  • since we want use grpc protobuf as the union type for different language, shall we also declare the error using protobuf? But seems like there is no standard Error syntax supported by protobuf. We need to create the error model ourselves. I am not quite familiar with this part, @huan could you please share some insights related to protobuf?

@huan
Copy link
Member

huan commented Mar 21, 2021

Discussion Draft

enum WechatyErrorType {
  UNKNOWN = 0,
  NO_PAYLOAD = 1,
}

puppet.on('error', data => {
  const e = new WechatyError(data)
})
class WechatyError extends Error {

  public static from (data: grpcError) {
    payload = JSON.parse(data)

    const e = new this(payload)
    return e
  }

  public to() {
    return {
      message: this.message,
      name: this.name,
      stack: this.stack,
      type: this.type,
    }
  }

  private type: WechatyErrorType

  private constructor (
    payload: any
  ) {
    this.type = WechatyErrorType[payload.type] ?? WechatyErrorType.UNKNOWN
  }
}

Google API Design Principle: Errors

@huan huan transferred this issue from wechaty/wechaty Oct 14, 2021
@huan huan added the enhancement New feature or request label Oct 14, 2021
huan added a commit that referenced this issue Oct 14, 2021
huan added a commit that referenced this issue Oct 15, 2021
* create PayloadCache class

* init mixins

* add mixin docs

* code clean

* change protected to jsdoc (#155)

* add CacheMixin & WatchdogMin with PuppetSkelton

* add CacheMixin & WatchdogMin with PuppetSkelton

* clean

* follow mixin design (#156)

* Create PuppetError classes (#159)

* split all code into mixins

* fix memory

* fix

* fix memory

* 0.49.4

* clean

* 0.49.5

* rename PuppetError -> GError

* rename PuppetError -> GError

* 0.49.6

* clean code

* 0.49.7
@huan
Copy link
Member

huan commented Oct 16, 2021

We have implemented the first version of a gRPC and ECMA compatible error class: GError.

Learn more at https://github.com/wechaty/puppet/blob/main/src/gerror/gerror.ts

huan added a commit to wechaty/wechaty that referenced this issue Oct 22, 2021
huan added a commit to wechaty/wechaty that referenced this issue Oct 23, 2021
#2275)

* add WechatyInterface & WechatyConstructor, as well as all User Class -es

* Interface all APIs

* 0.77.6

* change all Wechaty classes to interfaces

* 0.77.7

* move some of the user class to be Interface

* 0.77.8

* refactoring all user class to interfaces

* 0.77.9

* clean up

* 0.77.10

* add validation util

* 0.77.11

* use mixin to add valid api to all user classes

* 0.77.12

* only emit GError for error event in Wechaty (wechaty/puppet#159)

* 1. Add `wechaty.emitError(e: unknown)` to convert all errors to GError
2. Add `isTemplateStringArray` type guard (with unit test)
3. Add `Sleeper` user class to just sleep a while when it is being `say`-ed
4. Add `impl.*` export for user class implements
5. refactory `say()` code to keep DRY
6. code clean

* 0.77.13

* code clean

* 0.77.14

* add WechatyBuilder factory class

* add WechatyBuilder factory class

* 0.77.15

* clean

* 0.77.16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants