feat: polls rewrite #1373

MartinCupela · 2024-10-02T12:17:39Z

CLA

I have signed the Stream CLA (required).
Code changes are tested

Description of the changes, What, Why and How?

Changelog

…clarations

…e to vote_changed + vote_removed

…lls-rewrite

…or PollData only

…f multiple votes allowed

…lls-rewrite

arnautov-anton · 2024-10-25T15:54:41Z

src/poll_manager.ts

+  private subscribePollClosed = () => {
+    return this.client.on('poll.closed', (event) => {
+      if (event.poll?.id) {
+        this.fromState(event.poll.id)?.handlePollClosed(event);
+      }
+    }).unsubscribe;
+  };
+
+  private subscribeVoteCasted = () => {
+    return this.client.on('poll.vote_casted', (event) => {
+      if (event.poll?.id) {
+        this.fromState(event.poll.id)?.handleVoteCasted(event);
+      }
+    }).unsubscribe;
+  };
+
+  private subscribeVoteChanged = () => {
+    return this.client.on('poll.vote_changed', (event) => {
+      if (event.poll?.id) {
+        this.fromState(event.poll.id)?.handleVoteChanged(event);
+      }
+    }).unsubscribe;
+  };
+
+  private subscribeVoteRemoved = () => {
+    return this.client.on('poll.vote_removed', (event) => {
+      if (event.poll?.id) {
+        this.fromState(event.poll.id)?.handleVoteRemoved(event);
+      }
+    }).unsubscribe;
+  };


How come these are not handled within Poll instance itself? If it's loaded, it handles its own events, if it's not then it's not really necessary to care for it - no?

(including subscribePollUpdated)

That's a no-go, it was indeed our initial approach however there are a few issues with it:

Having 3000 polls essentially means having 3000 * 5 listeners at a certain point, all of which would be invoked when a new event arrives (and 2999 of them would simply skip because the event does not concern them)

It doesn't particularly scale well with more polls being loaded through pagination

This way, we only have a single set of listeners that consume one event at a time, updating the correct poll from within the cache directly (while maintaining referential integrity of the poll instances at all times)

So, in other words - they are indeed handled in the Poll instances themselves it's just not the Poll instances that listen to the events but rather the manager does.

So esentially - poll instance cannot properly live without the manager - correct? You always have to rely on manager to keep your poll instance up to date so "detached" instances are "broken" in a way.

I think it'd be good if we explored the idea of targeted events deeper - it should be very simple to do for polls as most of the events are poll-related and carry message/poll id's and we could benefit from this performance enhancement in the future. Poll could be fully encapsulated with logic it should "own" allowing it also to live in a detached (from the manager) state.

If I understand you correctly, you're looking for something like:

The pollManager is still listening for poll events

It re-dispatches a targeted event (for example, poll.vote_casted-<poll_id> or something`)

The poll itself is only listening to poll.vote_casted-<this.id> events and handles them

Is this correct ? I totally get your concerns and something like this was briefly considered too, however I do have some concerns about this approach too:

Each poll having its own listener can potentially be a ton of extra listeners

Not every listener would indeed be triggered (only the specific ones), but their event's dispatch would still be coupled with the poll manager, which would act as a router of sorts in order to orchestrate the traffic - doing these changes through events seems to me like a bit of an overhead considering that we can just update inline

If poll instances are indeed to listen, they have to be subscribed to events; since we need all of these to be up to date at all times we would basically need to subscribe to the new events once again whenever messages actually arrive within the state client.queryChannels or channel.query for example)

With that said, I do see some really good things with this as well:

The ability to hook up to specific poll events from anywhere (for example, if some custom integrator logic depends on events they can simply hook up to poll.vote_casted-<id> and not have to do an ID sanity check each time one of these events is fired)

Having them detached is a very, very good point too

So, I'm not particularly sure which way we should go - perhaps we should sit down and discuss this more thoroughly. Adding targeted events shouldn't be difficult as far as I can see (pretty much the same logic).

arnautov-anton · 2024-10-25T15:56:30Z

src/poll_manager.ts

+    return this.pollCache;
+  }
+
+  public fromState = (id: string) => {


Suggested change

public fromState = (id: string) => {

public fromCache = (id: string) => {

Deliberated quite a bit over this as well, but in the end I decided to go with fromState since the cache is the manager's state in some sense. It feels kind of wrong exposing something called fromCache to integrators since it might lead them to believe it's something else, but I might just be overthinking this :D

I'm worried that this might be a bit misleading either way as integrators might think that manager has a state which they can subscribe to as the API becomes the norm across the client though I understand your point too.

Maybe something like byId would also be good ? Not implying anything specifically that way, we're just saying that we're asking for a poll with a certain ID. What do you think ?

arnautov-anton · 2024-10-25T16:04:19Z

src/poll_manager.ts

+    return this.fromState(id);
+  };
+
+  public queryPolls = async (filter: QueryPollsFilters, sort: PollSort = [], options: QueryPollsOptions = {}) => {


StreamChat.queryThreads returns list of Thread instances - maybe we could do the same here for general API consistency?

The same goes for state hydration:

poll.hydrateState(differentPollInstanceButSameId)

Well, client.queryPolls should return the raw response in my opinion in case someone needs it (they'll have a way to do it). If they're reaching for reactive state though, they would fetch through pollManager.queryPolls(...) - this was the general idea. We can of course move the instantiation there too and simply make sure the cache is populated in pollManager, but it seemed like losing an API that way.

arnautov-anton · 2024-10-25T16:06:09Z

src/poll_manager.ts

+    const { poll } = await this.client.getPoll(id);
+
+    this.setOrOverwriteInCache(poll, true);
+
+    return this.fromState(id);


Why are we reaching for the request first instead of cache - I'd expect we'd be reaching for request in case of cache-miss?

Because we've decided that queryPolls and getPoll are always going to reinitialize the state within the cache. We want them to be used independently from the rest and be able to overwrite whatever's in the cache (not by reference though, only state) when they're queried. But, perhaps we can simply always use getPoll and provide a fallback for whenever it's not in the cache I suppose. Wanted to keep those 2 separate though.

There shouldn't be a need for it though, no? If the poll is in the cache then that means it's been receiving updates through event subscriptions meaning its state should closely represent the one that backend has - hydrating it at this point seems excessive.

The other way would be to return it right away and do the state hydration behind the scenes - since the state of the poll is reactive, it should not matter that it changes after it gets returned, no?

There is a particular need for this for offline storage in RN for example. Let us consider the following scenario:

You create a poll and are actively watching its progress (some votes, answers come in - w/e)

You close the app and do something else, while in the meantime more votes and/or answers arrive

You come back, and the first "optimistic" thing that loads is the data from the local DB (which is now out of date since those WS events never got consumed while you weren't using the app)

If you're offline, this is no biggie since we don't expect that WS events are handled in the background

If you're online however, the point at which a query is made for all of the channels (and their messages) you'll get all of the updated data; and it should indeed overwrite the local DB data (and store this agaiån)

Unfortunately, the sync API does not yet support poll events and so we can't particularly rely on it to do the heavy lifting for us. Whenever these potentially get added in the future, perhaps we can remove the overwriting mechanism (it's anyway hidden behind the second argument). For now, however - I believe we need it.

If it seems sketchy to you maybe we can lift the overwriting condition higher so that we can specifically trigger this only for the RN SDK (since the rest don't have offline support) ?

arnautov-anton · 2024-10-25T16:06:44Z

src/poll_manager.ts

+    const { poll: createdPoll } = await this.client.createPoll(poll);
+
+    return new Poll({ client: this.client, poll: createdPoll });


Why is the newly created poll not being cached?

It's not since presence in the cache is directly tied to the poll actually being sent as a message somewhere (i.e appearing when we queryChannels, or a message.new event arrives with a poll inside of it etc). But it might be a good point to perhaps not even create a poll instance, since in this case we're really only interested in the ID of the poll that got created.

MartinCupela and others added 30 commits September 18, 2024 11:05

feat: extract PollAnswer type from PollVote type

d4424ed

fix: populate missing generics for PollAnswer

b43297c

fix: populate missing generics for QueryPollsResponse

75f2b60

refactor: simplify PollAnswer type

9302884

feat: add client.queryPollAnswers method

28edc51

fix: remove channel and cid from PollResponse

dfa8f47

fix: fix poll types

a38f393

refactor: keep exports ordered in alphabetic order

fe845db

fix: add missing StreamChatGenerics to poll types in client method de…

7a523c6

…clarations

style: run lint

4151673

feat: add Poll class

a03a0c7

fix: remove paginator from index

5f0df10

feat: add Poll.queryPollAnswers & Poll.queryOptionVotes

c58d02b

fix: prevent recursion on poll.data getter

7d0206a

fix: remove poll event handlers in channel state

184cb80

feat: poll_manager initial impl

aece207

style: fix lint issues

1dfe2fe

feat: add maxVotedOptionIds, ownVotesByOptionId, ownAnswer to PollState

33cf859

style: fix lint issues

04efc24

fix: prevent executing poll event listener for other polls & subscrib…

51ac4f6

…e to vote_changed + vote_removed

fix: remove previous own answers on vote changed

33fd469

fix: make sure own votes do not contain answers

a3dacd0

fix: reassign own votes on vote change

9a53d6b

fix: override own votes on poll.vote_changed event

2c18517

fix: initiate ownAnswer in Poll state when initiating Poll instance

6cb6cc3

fix: minor issues and extract id from reactive state

f6b8b6d

fix: convert Poll methods to array functions

17a674f

Merge remote-tracking branch 'origin/feat/polls-rewrite' into feat/po…

49b420f

…lls-rewrite

fix: do not use poll.data.id

25a8ee3

fix: on poll WS event update partially the state with PollEnrichData …

eabf3e5

…or PollData only

isekovanic and others added 7 commits October 24, 2024 15:12

fix: populate poll cache on queryPolls and getPoll too

edd3f4d

chore: add more poll_manager tests

2deb62d

fix: lint issues

acb82cd

fix: update Poll.ownVotesByOptionId on poll.vote_changed by merging i…

c9aa7ea

…f multiple votes allowed

test: add Poll tests

bfb3e05

Merge remote-tracking branch 'origin/feat/polls-rewrite' into feat/po…

ec6dbee

…lls-rewrite

style: fix lint issues

4f8faec

arnautov-anton reviewed Oct 25, 2024

View reviewed changes

isekovanic added 3 commits October 28, 2024 18:23

Merge branch 'master' into feat/polls-rewrite

a2b7de1

chore: update from master and do PR remarks

7a506f3

fix: lint issues

ec3f3bf

MartinCupela marked this pull request as ready for review October 30, 2024 15:52

MartinCupela requested review from vishalnarkhede, santhoshvai, khushal87, szuperaz, oliverlaz and myandrienko as code owners October 30, 2024 15:52

MartinCupela requested a review from isekovanic October 30, 2024 15:53

fix: latest_answers resolution

b4082d8

isekovanic approved these changes Oct 30, 2024

View reviewed changes

fix: linter errors

f1610eb

isekovanic merged commit 7c56d21 into master Oct 30, 2024
5 checks passed

isekovanic deleted the feat/polls-rewrite branch October 30, 2024 16:09

github-actions bot mentioned this pull request Oct 30, 2024

chore: release v8.42.0 #1384

Merged

MartinCupela mentioned this pull request Oct 31, 2024

feat: add Poll support GetStream/stream-chat-react#2530

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: polls rewrite #1373

feat: polls rewrite #1373

MartinCupela commented Oct 2, 2024

arnautov-anton Oct 25, 2024

arnautov-anton Oct 25, 2024

isekovanic Oct 25, 2024

arnautov-anton Oct 25, 2024

isekovanic Oct 28, 2024

arnautov-anton Oct 25, 2024

isekovanic Oct 25, 2024

arnautov-anton Oct 25, 2024

isekovanic Oct 28, 2024

arnautov-anton Oct 25, 2024

isekovanic Oct 25, 2024

arnautov-anton Oct 25, 2024

isekovanic Oct 25, 2024

arnautov-anton Oct 25, 2024

isekovanic Oct 28, 2024

arnautov-anton Oct 25, 2024

isekovanic Oct 25, 2024

	public fromState = (id: string) => {
	public fromCache = (id: string) => {

		const { poll: createdPoll } = await this.client.createPoll(poll);

		return new Poll({ client: this.client, poll: createdPoll });

feat: polls rewrite #1373

feat: polls rewrite #1373

Conversation

MartinCupela commented Oct 2, 2024

CLA

Description of the changes, What, Why and How?

Changelog

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment