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

Deprecating poll #25

Open
dyoder opened this issue Feb 14, 2021 · 3 comments
Open

Deprecating poll #25

dyoder opened this issue Feb 14, 2021 · 3 comments

Comments

@dyoder
Copy link
Contributor

dyoder commented Feb 14, 2021

The polling combinators (poll, startPoll, stopPoll) belong at another layer of abstraction. They may also hint at some missing combinators in Carbon. For example, we can imagine implementing a simple variation of poll as:

activate [
  call -> @polling = true
  repeat [
    test (@polling), [
      call @poll
      sleep seconds: 5
    ] ] ]
deactivate [
  call -> @polling = false
]

The missing combinators are repeat, test, and sleep. Of course, this brings up the whole question of whether control flow should be done with combinators. We can do the same thing with code:

activate [
  call ->
    @polling = true
    loop
      if @polling
        @poll() 
        sleep seconds: 5
      else break
]
deactivate [
  call -> @polling = false
]

For that matter, we can do this with methods on Handle.

activate [
   call @startPolling
]
deactivate [
  call @stopPolling
]

The current implementation allows for multiple named polls. Perhaps @freeformflow can chime in here as to whether we had a scenario where we needed that or whether we were merely anticipating one. My current take on this is that if you need more than one poll, you perhaps need another component. But even if we did, that's state the client can be responsible for, or that we can introduce in another library. Carbon isn't meant to be quite that opinionated.

As far as idempotency, I don't think it's an issue here unless activate can be called without deactivate. This was a bit buggy before so perhaps that's why we felt we needed that. Again, maybe @freeformflow can remind me if I'm missing anything.

We can backport this code for clients that used it. If the additional complexity is warranted (named polls, redundant activation), we can move that into a separate module, ex carbon-polling or something like that.

dyoder added a commit that referenced this issue Feb 14, 2021
dyoder added a commit that referenced this issue Feb 14, 2021
@freeformflow
Copy link
Contributor

Regarding the multiple polls, I was being anticipatory. And for some reason, it was important to me to be able to give the polls names for future reference.

@freeformflow
Copy link
Contributor

Oh, it might have been idempotency that was at issue with the name?

@dyoder
Copy link
Contributor Author

dyoder commented Feb 14, 2021

Yeah, it also makes sense if you're maybe assuming that you call start/stop in response to other handlers. That's one advantage of just placing them inline: you're making it impossible to call them another way.

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

No branches or pull requests

2 participants