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

events? #643

Open
sonnyp opened this issue Apr 18, 2019 · 4 comments
Open

events? #643

sonnyp opened this issue Apr 18, 2019 · 4 comments

Comments

@sonnyp
Copy link
Member

sonnyp commented Apr 18, 2019

see #642 for background story

Similar story, I'm looking for feedback in how to design events in "higher level" packages such as roster and pubsub.

I'm not a big fan of using EventEmitter for this as we already have a middleware api that we could re-use. iqCallee is build on top and act as a router. It handles errors/exceptions automatically and its interface is compatible with servers.

There are 2 kinds of events in XMPP, events that require an ack (iq) and events that don't (message and presence).

Let's take roster as an example for the first, this is what it would look like with using iqCallee/middleware API:

const roster = createRoster({iqCallee})

// the package would make sure only roster push from server will be handled

// subscribe to roster push
roster.onSet((item, ctx) => {
  // ctx is from middleware package
  // item is from roster package
  item.attrs.jid
  item.attrs.ver
  // if you throw in here an iq type=error will be sent to the server thanks to iqCallee
})

roster.onRemove((item, ctx) => {
  // ctx is from middleware package
  // item is from roster package
  item.attrs.jid
  item.attrs.ver
  // if you throw in here an iq type=error will be sent to the server thanks to iqCallee
})

Let's take pubsub as an example for the second use case:

const pubsub = createPubsub({middleware})

// subscribe to retract notification
pubsub.onRetract((item, ctx) => {
  // ctx is from middleware package
  ctx.from
  // item is from pubsub package
})

// subscribe to publish notification
pubsub.onPublish((item, ctx) => {
  // ctx is from middleware package
  ctx.from
  // item is from pubsub package
})

cc @ggozad
cc @wichert

@wichert
Copy link
Contributor

wichert commented Apr 18, 2019

You definitely need the ability to both subscribe and unsubscribe at any point in time. That pretty much leads you to needing the equivalent of EventEmitter. So why not use that since it pretty much is an established standard?

@wichert
Copy link
Contributor

wichert commented Apr 18, 2019

One thing I'ld say is that plugins should not emit events through the client instance: that quickly can lead to naming conflicts.

@sonnyp
Copy link
Member Author

sonnyp commented May 16, 2019

You definitely need the ability to both subscribe and unsubscribe at any point in time.

I'm not convinced, do you have use cases in mind? XMPP is stateful, from experience this usually happens at protocol level. (Pubsub unsubscribe, presence unavailable, ...)

One thing I'ld say is that plugins should not emit events through the client instance: that quickly can lead to naming conflicts.

Yep definitely, that's how "plugins" are designed already, separate objects.

@wichert
Copy link
Contributor

wichert commented May 16, 2019

You definitely need the ability to both subscribe and unsubscribe at any point in time.

I'm not convinced, do you have use cases in mind? XMPP is stateful, from experience this usually happens at protocol level. (Pubsub unsubscribe, presence unavailable, …)

Yes: React components. They must unsubscribe when they are unmounted.

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

No branches or pull requests

2 participants