hubotio / hubot

A customizable life embetterment robot.
https://hubotio.github.io/hubot/
MIT License
16.64k stars 3.75k forks source link

Does `Response.envelope` really need include the `message` object too? #1659

Closed joeyguerra closed 11 months ago

joeyguerra commented 1 year ago
  constructor (robot, message, match) {
    this.robot = robot
    this.message = message
    this.match = match
    this.envelope = {
      room: this.message.room,
      user: this.message.user,
      message: this.message
    }
  }

The Response constructor sets envelope with the message but Response already has that message property. It's duplicated. Does it really need to be in envelope?

technicalpickles commented 1 year ago

The envelope is what is passed to adapters, ie: https://github.com/hubotio/hubot/blob/a4c2ec8fc2ba2bda378bebd35ed6fc1abe485689/src/response.js#L96

You'd have to take a close look at adapters in the repo and in the wild to see what they are doing, and the difference. I don't remember if I was around when it was added, but my intuition would be to keep it a simple javascript object that is the interface, rather than passing message classes around. That might be moot though, since message in turn is also included in it.

joeyguerra commented 1 year ago

I see your point. The envelope contains the original message object, and since the envelope is the thing that gets passed around, that's how other objects can get the message. I'll review other adapters to see how and if it's being used in the wild.

I agree with your point about keeping it a POJO instead of codifying it as a Class. Hubot, in general, is a message connector between two systems, so it makes sense to "just pass it through".