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

composition? #642

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

composition? #642

sonnyp opened this issue Apr 18, 2019 · 2 comments

Comments

@sonnyp
Copy link
Member

sonnyp commented Apr 18, 2019

While working on several "high level" packages for xmpp.js (pubsub, chat, ...) I realized there was one use case the current architecture does not help with.

Let's take PubSub and RSM

RSM is not limited to PubSub and PubSub does not need RSM, as such I do not want to implement RSM directly in the pubsub package. So I'm not sure how to offer RSM features to users.

I think composition would be great for this use case but I'm not sure how we should implement this

example:

const result = await pubsub.get(withRSM(<myquery/>, {max: 8}))

However it does not provide enough flexibility, wouldn't be able to return the rsm info or iterate over them.

Maybe we could make plugin expose their internal functions to create stanzas and do

const request = pubsub.createGetStanza(...)

for await (const paginatedResult of rsm.paginate(request, {max: 10}) {
  console.log(paginatedResult)
}

// paginate() returns an asyncIterator

Thoughts? Comments?

cc @ggozad
cc @wichert

@sonnyp sonnyp mentioned this issue Apr 18, 2019
@sonnyp sonnyp closed this as completed Apr 18, 2019
@sonnyp sonnyp reopened this Apr 18, 2019
@wichert
Copy link
Contributor

wichert commented Apr 18, 2019

Requiring generators in an API feels premature - support for them may not be perfect, and they certainly make debugging transpiled code very painful.

createGetStanza feels like something that is too low-level for most code. I think there should be some goals for an API:

  1. a normal non-RSM call should be simple, and not require two steps of creating a stanza and then submitting it
  2. non-RSM call should return the same type of result as a call to a non-RSM capable entity
  3. doing a call with RSM support should be as simple as possible and not require people to learn new concepts
  4. it would be nice if non-RSM and RSM calls had the exact same return type. This makes type definitions and cognitive load simpler.
  5. the result for an RSM call should be something that allows standard RSM functionality:
  • get the total number of results
  • get the next batch
  • get the previous batch
  • get the last batch
  • get the first batch
  • get a random batch number

This suggests that a call should always return a wrapper / proxy with an RSM-capable interface. Something like:

interface RSMParameters {
    max?: number
    before?: string
    after?: string
    index?: number
}

interface RSMResult<T> {
    supportsRsm: true
    data: T
    count: number,
    first: string,
    last: string
    previous: () => Promise<RSMResult)
    next: () => Promise<RSMResult>
    get: (q: RSMParameters) => Promise<RSMResult>
}

interface NonRSMResult<T> {
   supportsRsm: false
   data: T
}


type PubSubResult = XML // I don't know from memory what this exactly looks like

interface PubSubPlugin<T> {
    get: (query, rsm: boolean | RSMParameters=false) =>
         Promise<RSMResult<PubSubResult>|NonRSMResult<PubSubResult>> 
} 

As an alternative we could return the data directly if rsm=false. I might actually prefer that. The API would then become:

interface PubSubPlugin<T> {
    get: (query, rsm: boolean | RSMParameters=false) =>
         Promise<RSMResult<PubSubResult>|PubSubResult> 
} 

@ggozad
Copy link
Member

ggozad commented Apr 23, 2019

Hey,
RSM is indeed a kind of a special case as it works on different contexts.
I don't think it's possible to compose with RSM and keep things simple for plugin authors.
I would build it in in the plugins that need it. I can't think of any other than disco and search and even these scenarios are fringe.
I would be happy with what wichert suggests, it keeps the API simple and is essentially what we are doing already except with the different returns.

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

3 participants