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/eventemitters #35

Merged
merged 23 commits into from
Apr 10, 2019
Merged

Feature/eventemitters #35

merged 23 commits into from
Apr 10, 2019

Conversation

RLesur
Copy link
Owner

@RLesur RLesur commented Apr 6, 2019

In this PR, we introduce the EventEmitter class. This class is inspired from the node.js EventEmitter class as mentioned in #8

R/eventemitters.R Outdated Show resolved Hide resolved
}
)
)

once_function <- function(fun) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Out of curiosity, why the need of once_function ? For adding a specific class to fun ?
I thought removing the listener was a guaranty of running it once. 🤔

Copy link
Owner Author

Choose a reason for hiding this comment

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

I've played a lot in node.js with the events module. I wanted to understand some behaviors.
More precisely, when I ran those examples, I realized that rawListeners just run once. This was not the case with our current implementation.
In node.js, rawListeners are listeners wrapped in a once wrapper object.

@cderv
Copy link
Collaborator

cderv commented Apr 8, 2019

Nice additions ! Tell me when you are ready.

FWIW, I planned to add the complete https://nodejs.org/api/events.html spec if event emitter was interesting to us. I currently just use what was interesting for crrri. I would have wait for #29 for a complete R API.

@RLesur
Copy link
Owner Author

RLesur commented Apr 8, 2019

I am not ready yet. I wanted to be sure that we were in the right direction. After carefuly reading the specs, I have a serious doubt.
I think that with the Callbacks class of rstudio/websocket, we will struggle to implement the prependListener and prependOnceListener methods.
Now, I'm convinced that at one time, we will have to implement a custom class for dealing with that and remove the code from rstudio/websocket.
So, I wonder whether it is worth to integrate now these code from rstudio/websocket and change the License.
I have an idea of implementation for another class that would support a prepend method.
I would prefer to test this idea before merging this PR, change the License, and so on...

@cderv
Copy link
Collaborator

cderv commented Apr 8, 2019

Ok I see. You went further than I expected. ☺️

Initially, I work on this to provide the necessary base for creating CDPSession in order to have a working POC to decide if this API was useful. I did not put all energy to make a complete and thorough rewrite of event emitter class.

As you want to go deeper, I believe you find it useful 😄

Rewriting this sounds like a good idea. I also have other ideas as I limit myself in the first round. It could be cleaner to rewrite in another branch maybe. (not sure 🤔 )

Are you planning on going further on this before looking to the upper layer (CDPSession) to have the bigger picture ?

@RLesur
Copy link
Owner Author

RLesur commented Apr 8, 2019

I will give my idea a try during traveling tonight (2 hours, sounds like a good timeout for a try). I let you know after that.

@RLesur
Copy link
Owner Author

RLesur commented Apr 8, 2019

I like working in the 🚄 (except for the connexion).
The problem of {websocket} Callbacks class is that it does not handle well a queue (we cannot prepend an element).
So, here's a proposal of an R6 class Queue.
The main difference is that Queue is generic (it handle any object): so, there is no invoke methods (not a problem for me). Otherwise, it has a similar behavior. The main advance is that with Queue, we can prepend an element:

Queue <- R6::R6Class(
  "Queue",
  private = list(
    .queue = list(),
    .wrap = function(element) {
      wrapper <- new.env(parent = emptyenv(), size = 1L)
      wrapper$element <- element
      wrapper
    },
    .rm_wrapper = function(wrapper) {
      element <- wrapper$element
      queue <- private$.queue
      # Since wrappers are environments, identical() will always send back at most one element.
      # This is the main trick.
      pos <- Position(function(x) identical(x, wrapper), queue)
      queue[pos] <- NULL
      private$.queue <- queue
      element
    }
  ),
  public = list(
    append = function(element) {
      wrapper <- private$.wrap(element)
      private$.queue <- c(private$.queue, list(wrapper))
      function() {private$.rm_wrapper(wrapper)}
    },
    prepend = function(element) {
      wrapper <- private$.wrap(element)
      private$.queue <- c(list(wrapper), private$.queue)
      function() {private$.rm_wrapper(wrapper)}
    },
    get = function() {
      lapply(private$.queue, function(w) get("element", pos = w))
    },
    remove_element = function(element, right = TRUE) {
      queue <- private$.queue
      pos <- Position(function(x) identical(x$element, element), queue, right = right)
      queue[pos] <- NULL
      private$.queue <- queue
      element
    }
  )
)

My idea is to use Queue instead of Callbacks. The invoke code would be moved inside EventEmitter$emit() method.
What do you think of that?
If we don't use Callbacks, all the code will be from us and there would be no need to change the License.

@RLesur RLesur merged commit 53da542 into master Apr 10, 2019
@RLesur RLesur deleted the feature/eventemitters branch April 10, 2019 16:41
@cderv
Copy link
Collaborator

cderv commented Apr 11, 2019

It is merged ! Thanks! I hope my previous work help you for the rewrite ! 😉
Good implementation !
Let's continue with CDP session now.

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