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

Feature/fb message types #115

Merged
merged 8 commits into from
Apr 25, 2018

Conversation

SilverFox70
Copy link
Contributor

As of May 7, 2018, Facebook is requiring that all messages sent to Messenger include messaging_type. This branch includes support for this change as well as support for including message_tag; although it does not specifically throw an error if someone attempts to send a message without the messaging_type field.

Spec files (facebook-format-message-spec.js and facebook-reply-spec.js) have been updated to test these changes and all tests are passing.

@stojanovic stojanovic self-assigned this Apr 24, 2018
Copy link
Member

@stojanovic stojanovic left a comment

Choose a reason for hiding this comment

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

Most of the things looks ok. One question: as message type is required, why don't we set it to "RESPONSE" as default value and allow users to change it with methods you provided?

'FEATURE_FUNCTIONALITY_UPDATE', 'TICKET_UPDATE'];

const includes_polyfill = require('../utils/includes-polyfill');
includes_polyfill();
Copy link
Member

Choose a reason for hiding this comment

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

This is nice, but I don't think we should include 50+ lines polyfill when we can use indexOf, can you please remove the polyfill and replace includes with indexOf?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair points. I'll make the changes and update.

@stojanovic stojanovic merged commit 245cb53 into claudiajs:master Apr 25, 2018
@stojanovic
Copy link
Member

lgtm, thanks!

@SilverFox70
Copy link
Contributor Author

SilverFox70 commented Apr 25, 2018 via email

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