Change instanceof to respect interfaces

[fcsonline]

Hubot is a plugin/adaptor framework that hosts them as node modules.
Those modules have their own dependencies versions. By depending on
types rather than interfaces, you run into the exact issue caused by
peerDependency explained here

[fmvilas]

👍

[enricostano]

👍

[edulan]

👍

[saimonmoore]

This makes so much sense. 💯

[michaelansel]

This should probably be 'catchall' or something a little more specific than 'all'

[fcsonline]

ack

[michaelansel]

Added comments on the code. I'm not sure I fully understand the need for this change though... The linked npm issue is pretty meaty; I'll try to read through it and wrap my brain around the change, but a summary of what is going on would be immensely helpful.

[fcsonline]

It's a really long story around dependencies and plugins like hubot does. If you add instanceof checks then you hit a big issue for the plugin development. I tried to explain the problem in the PR description.

Also it's related with gotchas section in https://hubot.github.com/docs/adapters/development/

[michaelansel]

Code looks good, I still want to understand the problem before giving a 👍 though. I'll try to find some time to noodle on it next week after Velocity. Ping me if I haven't gotten back to this by the end of next week.

[fcsonline]

Ok! No problem!

[bkeepers]

For consistency?, should this be if message.type is 'text'

[technicalpickles]

I'd be interested to know how (if at all) this affects adapters that create their own subclasses of Message, like slack does:

• https://github.com/slackhq/hubot-slack/blob/master/src/message.coffee
• https://github.com/slackhq/hubot-slack/blob/master/src/listener.coffee

I've tried thinking through it a few times, and didn't spot any obvious problems, but I'd like to get it written out at some point.

[technicalpickles]

@fcsonline there was a bit of work done last week on master so there's a conflict now. Would you mind merging in master and getting an update pushed here? Should be good to merge after that.

[michaelansel]

Unless this is fixing a bug/preventing other work, I'd rather have a good understanding of why this is needed before it merges. If you understand it though, @technicalpickles, then go for it. We also might want to add documentation somewhere of the different message types and what they mean (so that new message classes have an idea of what type to use).

[michaelansel]

Okay, did some more noodling and testing. I didn't realize that instanceof only looks at the immediate parent and not the full inheritance hierarchy:

class Message
  @type = 'message'

class TextMessage
  @type = 'text'

msg = new Message()
tmsg = new TextMessage()

# tmsg instanceof TextMessage # true
# tmsg instanceof Message # false

This led me to a different question though: why are we ever testing message types? Is the only reason we care for Listener matching?

My answer: yes. So, therefore message types are a way to bucket Message objects for selection by Listeners.

This would result in the following mapping:

Message Type	Listener "Types"
text	hear, respond, eavesdrop
enter	enter
leave	leave
topic	topic
catchAll	catchAll, catchAllAddressed

Then, any time a new Message subclass is created, it needs to specify a type that will associate it with one of the above Listener types.

If we do this, are there then certain expectations around how a certain Message subclass behaves? If so, we need to call those out. As I see it right now, the only expectation is:

• a text message should have a match method that takes a regular expression

Finally, it looks like the TextListener class is the only thing imposing requirements on Message types.

I won't (yet) go so far as to make a recommendation (want to noodle more), but that's what I've got in my head so far.

[fcsonline]

@michaelansel, for this reason I more confortable doing duck typing checks instead of instanceof for TextListener.

[michaelansel]

for this reason I more confortable doing duck typing checks instead of instanceof for TextListener.

After thinking about this some more, I'm inclined to agree with you now. As an aside, I'm thinking about nuking TextListener as a follow up to #986, but that needs some more thought.

I'm still trying to find time to really think hard about this, but I imagine it will be a little while before this gets to the top of my queue.

[sergeyt]

👍

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.