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

Pass mention list up and down #34

Merged
merged 7 commits into from
Nov 27, 2018
Merged

Pass mention list up and down #34

merged 7 commits into from
Nov 27, 2018

Conversation

windmemory
Copy link
Member

Refer to issue: wechaty/wechaty#1560

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.

Please keep the naming style consistent.

src/puppet.ts Outdated Show resolved Hide resolved
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.

Please keep the schema protection in your modification.

toId? : string, // if to is not set, then room must be set
}

/** @hidden */
export interface MessagePayloadTo {
Copy link
Member

Choose a reason for hiding this comment

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

Please do not simply merge those interfaces. They are separated by design.

We need to make sure that: either fromId or roomId must exist

The original design is to protect those data schema; please do not remove this protection.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, if that's the case, we should have no roomId in the MessagePayloadTo, and no fromId or toId in the MessagePayloadRoom, right?

Could you please explain a little about the reason that we have that at the first place?

Copy link
Member Author

Choose a reason for hiding this comment

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

According to current design, maybe you will agree with this design?

export interface MessagePayloadBase {
  id            : string,

  // use message id to get rawPayload to get those informations when needed
  // contactId?    : string,        // Contact ShareCard

  filename?     : string,
  text?         : string,
  timestamp     : number,        // Unix Timestamp(in seconds)
  type          : MessageType,
}
/** @hidden */
export interface MessagePayloadRoom {
  fromId?       : string,
  mentionIdList?: string[],   // Mentioned Contacts' Ids
  roomId        : string,
  toId?         : string,   // if to is not set, then room must be set
}
/** @hidden */
export interface MessagePayloadTo {
  fromId        : string,
  mentionIdList?: string[],   // Mentioned Contacts' Ids
  roomId?       : string,
  toId          : string,   // if to is not set, then room must be set
}
export type MessagePayload = MessagePayloadBase
                            & (
                                MessagePayloadRoom
                              | MessagePayloadTo
                            )

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.

Sorry but I don't think this is right: the MessagePayloadTo should never have the mentionIdList property.

@windmemory
Copy link
Member Author

Then why it has roomId originally? That doesn't make sense. Also why MessagePayloadRoom has toId? Is there any reason for that?

@huan huan merged commit 06bef87 into wechaty:master Nov 27, 2018
@windmemory
Copy link
Member Author

So what was the reason that we have the roomId originally? Is that for making typescript happy? Or some other reason? I spent a lot of time trying to make it work with the design that only have roomId and mentionIdList on the MessagePayloadRoom and only have toId and fromId on the MessagePayloadTo, but haven't figured out the right answer to that. I am really curious about the original design and how to make it work.

@huan
Copy link
Member

huan commented Nov 27, 2018

To be honest, I forgot.

The only thing that I had remembered is that it was designed for a purpose.

Would love to discuss it with you when we got time, but let's just move forward and leave this as it is.

@windmemory
Copy link
Member Author

Cool, let's talk then.

And thanks for merging this PR :)

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