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/cdp session with eventemitter #38

Merged
merged 55 commits into from
Apr 14, 2019

Conversation

RLesur
Copy link
Owner

@RLesur RLesur commented Apr 10, 2019

I open this PR now in order to have a room for discussion for this branch.

cderv and others added 30 commits January 1, 2019 20:22
this avoir infinite recursion with newListener for example
and add debug code to help diagnosis
Merge branch 'master' into feature/CDPSession-with-eventemitter

# Conflicts:
#	R/eventemitters.R
#	man/EventEmitter.Rd
@RLesur
Copy link
Owner Author

RLesur commented Apr 12, 2019

I am really embarrassed by

# if a reponse to a command, emit the callback corresponding to the id
if (!is.null(id)) {
method_sent <- private$.commandList[[self$id]]$method
private$.commandList[[self$id]] <- NULL
self$emit(method_sent, data)
}

There are two kinds of messages sent by Chrome:

  • Chrome events
  • Chrome responses (to commands)

With this implementation, the EventEmitter object throws the same kind of events for Chrome events and Chrome responses: by listening the EventEmitter object, we cannot distinguish Chrome events and Chrome responses . I understand your goal but I fear that could lead to some errors.

I would prefer to keep the distinction between Chrome events and Chrome responses: I will propose some changes in this direction.

@cderv
Copy link
Collaborator

cderv commented Apr 12, 2019

It was my understanding that only commands have id. These lines are just emitting the event corresponding to the command sent. Lines after are responding the same way but to method.
What difference would like to have between those two?

@RLesur
Copy link
Owner Author

RLesur commented Apr 14, 2019

What do you think of that?
I used many events from the chrome-remote-interface library: connect, ready...
I also created new events that would be useful to implement the promises API on top of CDPSession.

@cderv
Copy link
Collaborator

cderv commented Apr 14, 2019

Definitely looks good ! It is fine by me.
Should we merge and see how it goes from there with example and test run ?

@RLesur RLesur merged commit 7886a7b into master Apr 14, 2019
@RLesur RLesur deleted the feature/CDPSession-with-eventemitter branch April 14, 2019 19:40
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