Skip to content
This repository has been archived by the owner on Jul 9, 2022. It is now read-only.

Added support for MQTT #765

Merged
merged 10 commits into from
Dec 2, 2019
Merged

Added support for MQTT #765

merged 10 commits into from
Dec 2, 2019

Conversation

HossamMohsen7
Copy link
Contributor

@HossamMohsen7 HossamMohsen7 commented Nov 27, 2019

Added support for MQTT with the existing events and messages.
Also added support for marking a message as delivered because I think that causes facebook to temporarly block accounts.
Also added a "change_thread_image" event triggered when a group image is changed.

To listen with mqtt simply change api.listen to api.listenMqtt

@Schmavery
Copy link
Owner

Wow this is great, thanks.

Can we mark-as-delivered automatically? Generally I'd say that that and change_thread_image could have been in a separate PR though.

I left a couple questions in the code, but it generally looks like it could work.
If you can add a brief note to the docs about the listenMqtt function being an experimental way to receive messages and a small usage example, I think we can probably merge this. If it works well for people, we can transition in a while to having it be the default listen behaviour. I'm assuming the messages that we get through this are pretty much the same as the ones we used to get through the regular listen?

In the meantime, if anyone else who is excited about this functionality wants to pull the branch and try it out, that's always helpful for a big change like this. Thanks!

src/listenMqtt.js Outdated Show resolved Hide resolved
src/listenMqtt.js Outdated Show resolved Hide resolved
@HossamMohsen7
Copy link
Contributor Author

Sure. We also could add an option for auto marking messages as seen.
for change_thread_image I could do that in another PR with some other events I found related to calling.

And yes, messages are exactly the same.
Thanks!

@HossamMohsen7
Copy link
Contributor Author

Also it would be helpful to merge #628 because of ForcedFetch.

@ravkr
Copy link
Contributor

ravkr commented Nov 28, 2019

I'd suggest changing tabs to spaces, to match rest of the code.

@HossamMohsen7
Copy link
Contributor Author

@Schmavery Would you like me to remove markAsDelivered and change_thread_image from this PR and do another one after this is merged?

src/mqtt/fbws.js Outdated Show resolved Hide resolved
src/mqtt/fbws.js Outdated Show resolved Hide resolved
src/mqtt/fbws.js Outdated Show resolved Hide resolved
@Schmavery
Copy link
Owner

Schmavery commented Nov 30, 2019

I don't really mind change_thread_image staying but lets pull out markAsDelivered for now if you don't mind -- I think we should start trying to do that and markAsRead automatically and I don't want that to block this PR

Thanks for adding the docs! I left just a couple more comments and then I think we should be good to merge -- looks like several people in the other thread are successfully able to use this code so great job :)

Also, I left a comment on #628 -- I'm not against merging something like this if it helps, but that particular PR is unfortunately out of date and a bit hard to review.

@HossamMohsen7
Copy link
Contributor Author

Thank you! I'm glad I could help.
I have no problem in removing markAsDelivered for now, I can work on making it automatic with markAsRead.

PR #628 has a good way to handle GraphQL requests that way most requets are in one place.
I could try to merge it and make it up to date if you like.

I'm also thinking about refactoring parseDelta so we could add more events in the future easily. But I'll leave that for another PR.

Thanks!

@HossamMohsen7
Copy link
Contributor Author

Could you please take a look at the latest changes?

@BadAimWeeb
Copy link
Contributor

image
:|

@ravkr
Copy link
Contributor

ravkr commented Dec 1, 2019

@lequanglam what was in that message? was it normal text? emoji? sticker? picture? that would probably help

@BadAimWeeb
Copy link
Contributor

BadAimWeeb commented Dec 1, 2019


@ravkr i can't check what messages is throwing that error (cuz there's a LOT of messages going in and out), but i hooked up some error logging, hopefully it will catch what messages is messing up.

image

địt mẹ facebook

@BadAimWeeb
Copy link
Contributor

BadAimWeeb commented Dec 1, 2019

found, it was ForcedFetch that cause issues.
image
@ravkr @ivankolesnikov @Schmavery
#628

var presence = {
type: "presence",
userID: userID,
timestamp: data["l"],
Copy link

Choose a reason for hiding this comment

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

I haven't seen this field in any of my orca_presence messages, but there's one called "c" that's still a mistery to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I'm trying to figure out what that is for. I'll update if I found something.

@HossamMohsen7
Copy link
Contributor Author

HossamMohsen7 commented Dec 1, 2019

@lequanglam It's fixed in the last commit. Please update.

Edit: The error I fixed is different from yours. I'll push a commit as soon as I can.

Copy link
Owner

@Schmavery Schmavery left a comment

Choose a reason for hiding this comment

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

This looks fine to me, @7osCraft please let me know when you're ready to merge it (from reading the comments it looks like you're still fixing one or two bugs?)

@HossamMohsen7
Copy link
Contributor Author

Fixed ForcedFetch issues and converted presence timestamps from seconds to milliseconds.
You can merge now. 😃

@Schmavery
Copy link
Owner

Thanks :)

@Schmavery Schmavery merged commit 22d1424 into Schmavery:master Dec 2, 2019
@PixelHir PixelHir mentioned this pull request Dec 6, 2019
@thanhdatpd
Copy link

thanhdatpd commented Dec 8, 2019

Hi @7osCraft
Sometime mqtt client has this error or after call api.logout()
image
listenMqtt just show it and nothing trigger. Can you add some error return like this to handle easy.
image
Thanks and sorry for my English.

@HossamMohsen7
Copy link
Contributor Author

@thanhdatpd Sure. Thanks!

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

Successfully merging this pull request may close these issues.

6 participants