Skip to content
This repository has been archived by the owner on Sep 20, 2024. It is now read-only.

Add a type field to message to distinguish between 'message_received' and 'postback' #469

Merged
merged 4 commits into from
Nov 7, 2016

Conversation

agamrafaeli
Copy link
Contributor

Apropo to issue #447

Add a type field to message when sending to receiveMessage() with a different value when the message is received on 'message_received' and when it is received on 'postback'
@ianp
Copy link

ianp commented Nov 1, 2016

See also PR #448 although the two are almost identical!

I think that I like my approach of reusing facebook_postback vs. postback for that type, but I also like that you've added a type field to non-postback messages, saves a null-check in client code.

@agamrafaeli
Copy link
Contributor Author

I agree re-naming convention. Personally I have an aversion to fields that require a null check.

From what I can tell receiveMessage() is only called twice - on message_received and on facebook_postback. Thus adding type on both of them resolves the issue.

@agamrafaeli
Copy link
Contributor Author

This may be over-engineering at this point, but maybe they should both be string constants?

@ianp
Copy link

ianp commented Nov 2, 2016

I like to use string constants for stuff like this in my own projects, but it's not a common approach in JS land generally.

@agamrafaeli
Copy link
Contributor Author

So are we in agreement on the solution here? Do we still need two PRs?

@ianp
Copy link

ianp commented Nov 3, 2016

No, I've closed the other one, let's get this one merged 😀

@agamrafaeli
Copy link
Contributor Author

Sounds good to me. Got any info on the frequency of merges here?

@ianp
Copy link

ianp commented Nov 4, 2016

Looking at the number of open PRs and issues, not high! But I'll mention it in the Slack channel.

@ianp
Copy link

ianp commented Nov 4, 2016

In the doc changes you have “type will be set to postback” when it should say facebook_postback.

BTW talking with the Howdy guys now about getting some open PRs merged.

@agamrafaeli
Copy link
Contributor Author

@ianp changed. Good catch.

@ianp
Copy link

ianp commented Nov 4, 2016

This looks good to merge now.

@benbrown benbrown merged commit 689ae7c into howdyai:master Nov 7, 2016
@benbrown
Copy link
Contributor

benbrown commented Nov 7, 2016

Thank you guys! Sorry it took longer than it should have. Merged!!

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

Successfully merging this pull request may close these issues.

4 participants