Skip to content
This repository was archived by the owner on Aug 31, 2018. It is now read-only.

Adds ISubscription and ISubscriptionRead - #53

Open
marceloschmidt wants to merge 4 commits into
masterfrom
subscription-read
Open

Adds ISubscription and ISubscriptionRead#53
marceloschmidt wants to merge 4 commits into
masterfrom
subscription-read

Conversation

@marceloschmidt

Copy link
Copy Markdown
Member

No description provided.

* @param roomId the id of the room
* @returns the subscription
*/
getByRoomId(roomId: string): Promise<IterableIterator<ISubscription>>;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you help me understand why you're returning an IterableIterator here when you're only returning one? https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Iteration_protocols

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

the comment is wrong, it's supposed to return an Iterable of ISubscription

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Okay, so maybe the one above it should read Gets all of the subscriptions which are for the provided room. as the gets a subscription is a little misleading as well.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Perfect! Thanks!

@geekgonecrazy

Copy link
Copy Markdown
Contributor

This gets the room subscriptions right? So returns all users subscriptions to a room?

Would RoomSubscription be more clear from an outside developers perspective?

@graywolf336

Copy link
Copy Markdown
Contributor

@geekgonecrazy I think I would tend to agree as well, Subscriptions is very generic. I know that right now Rocket.Chat only has room subscriptions, but what if in the future we add more? Thus, it would be nice to have it be very specific instead of generic. RoomSubscription sounds good 👍

@geekgonecrazy

Copy link
Copy Markdown
Contributor

Yes not to mention meteor subscriptions etc. It would be great from an apps perspective if that was never an issue that ever needed clarified

@marceloschmidt

Copy link
Copy Markdown
Member Author

And should we then also change AppSubscriptionBridge and AppSubscriptionsConverter inside rocketchat-apps package to RoomSubscription?

@rodrigok

Copy link
Copy Markdown
Member

Please move this Pull Request to the repository https://github.com/RocketChat/Rocket.Chat.Apps-engine since we moved this code to https://github.com/RocketChat/Rocket.Chat.Apps-engine/tree/master/src/definition

Thanks

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants