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

Discussion: do we need a new event when sync contacts done #7

Closed
windmemory opened this issue Jul 9, 2018 · 17 comments
Closed

Discussion: do we need a new event when sync contacts done #7

windmemory opened this issue Jul 9, 2018 · 17 comments

Comments

@windmemory
Copy link
Member

windmemory commented Jul 9, 2018

I would like to discuss whether we need a new event, emitted when we sync all the contacts after login.

I encountered this problem when I was trying to write the integration tests. I want to do my tests after the contacts are all loaded, but found it is pretty hard to get to that point. I can write tests in login event, but during that time, the contacts are not loaded.

Reason I think we should have this event:
Give the devs an entry point when the contacts are all fully loaded, otherwise, if the developer try to find a contact, but the contact is not fully loaded, and the contactList is from our cache, so the returned contacts collection to be searched is not in ready state. Then the searchContact will fail.

@zixia

@huan
Copy link
Member

huan commented Jul 9, 2018

I would like to think about your proposal after the publish of Wechaty v0.18 because currently, we have too many more important BUGS to fire.

Before that, I believe you can solve this problem by mocking the Sync Data.

@windmemory
Copy link
Member Author

Okay, I got that. Mocking SyncData is not hard, the hard thing is I need to find a way to detect when the sync data finished. I will try to find a way or get a work around.

But please leave this issue open, so we can discuss this after the publish of v0.18 :)

@windmemory
Copy link
Member Author

I would like to re-active this discussion thread since we are receiving developers asking for right way to get the room members. Refer here

I think if we have an event telling the developers that the Rooms and Contacts are finished loading, they would not have this problem.

Currently their alternative way of getting the whole room information is that they do Room.findAll() twice, and compare the results between these two, if they have the same count, that possibly means the loading is finished, but this is a hack way of doing this, since we know when the synchronize is done inside wechaty, it makes sense to emit an event out to our developers.

@zixia If you agree with this idea, I would send out PR later.

@huan
Copy link
Member

huan commented Jul 24, 2018

Understand.

I think you can send a workable PR to demonstrate your implementation, then we could be able to discuss base on that.

@windmemory
Copy link
Member Author

Hey @zixia , here is the c9 link: https://ide.c9.io/lijiarui/sync-done-event-test

Please take a look and let me know what do you think.

@huan
Copy link
Member

huan commented Jul 24, 2018

Ok, I will go to have a look.

But I need you to send the PR for the related source code before I can involve.

@windmemory
Copy link
Member Author

Could you please add me as collaborator of wechaty and wechaty-puppet, then I can create new branch on the repo instead PR from fork, so in the mean time, it is also more convenient for you to edit based on my code.

@huan
Copy link
Member

huan commented Jul 24, 2018

Sorry, I'm afraid that I can not add you for now because all the repo is in DevOps mode and it will be very dangerous if we made any mistake.

So I'm sorry that you have to use the standard fork/pr for our collaboration, and you need not worry about me: I can edit based on your code at any time after you sent PR.

@windmemory
Copy link
Member Author

@huan
Copy link
Member

huan commented Jul 25, 2018

I'm thinking about the event name. It is very important because your PR will introduce a new event to Wechaty, which is a big decision.

I do not satisfy with data-ready, and I'm thinking about to use ready instead.

However, the name ready make me feels not good too because it is too general...

Do you have any better idea about the event name?

@windmemory
Copy link
Member Author

I agree with you about the event name. I am also not satisfied with the data-ready, it was just for prototype, so I used this name.

Originally I was thinking about sync-done, but I am not 100% satisfied with the name.
Maybe we could ask the community about the name?

@huan
Copy link
Member

huan commented Jul 25, 2018

How do you feel about wechaty.on('ready')? I can not find any alternate name except the ready right now.

@windmemory
Copy link
Member Author

As you said, I also think it is too general. If syncRoomAndContacts done means the whole wechaty is finally ready, then ready is okay.

What do you think about in-sync? Means wechaty's data is in sync with remote.

huan pushed a commit to wechaty/wechaty that referenced this issue Jul 26, 2018
* update padchat version

* 0.17.119

* prototype for room-invite event

* Revert "prototype for room-invite event"

This reverts commit 33c4321.

* add data-ready event

* add `ready` event and ready() method (wechaty/puppet#7)

* make lint happy

* 0.19.113
@huan
Copy link
Member

huan commented Jul 26, 2018

I decide to use ready as the event name at last, because:

  1. it's the best name to tell the developer that the Wechaty is all ready to use
  2. it does not mean sync-ready, it just mean ready. So every puppet implementation can define what's the meaning of the ready, and emit ready whenever needed.

The ready event should be ready since the version from:

  1. wechaty@0.19.113
  2. wechaty-puppet@0.9.22

Usage

1. @event ready

You must add the listener to the ready event of Wechaty before start(). If the Wechaty is already ready, and you add a ready event listener after that, then the listener is mostly like to be never called because the event had already been fired.

wechaty.on('ready', () => {
  console.log(`I'm ready`)
}

2. ready() method

You can call wechaty.ready() at any time and it will return a Promise to you. If the Wechaty is ready, the Promise will be solved at once and you can do whatever you want with the ready-ed Wechaty.

wechaty.ready().then(() => {
  console.log(`I'm ready`)
})

@windmemory
Copy link
Member Author

You must add the listener to the ready event of Wechaty before start(). If the Wechaty is already ready, and you add a ready event listener after that, then the listener is mostly like to be never called because the event had already been fired.

That is to say, it is encouraged to do chaining when creating bot with wechaty?
If I do

bot
.on('ready', () => console.log('wechaty is ready')
.start()

The ready event will promised to be triggered, right?

@huan
Copy link
Member

huan commented Jul 26, 2018

Correct.

@windmemory
Copy link
Member Author

Okay, got it, thanks for make this implemented such soon. 👍

huan pushed a commit to wechaty/wechaty that referenced this issue Aug 6, 2018
* update padchat version

* 0.17.119

* prototype for room-invite event

* Revert "prototype for room-invite event"

This reverts commit 33c4321.

* add data-ready event

* add `ready` event and ready() method (wechaty/puppet#7)

* make lint happy

* 0.19.113

* add method on contact-self to update name and signature

* change log and add comments for new method

* code clean

* 0.19.125
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

No branches or pull requests

2 participants