ssbc / scuttle-poll

3 stars 1 forks source link

Adds `poll.obs.get` #24

Closed pietgeursen closed 6 years ago

pietgeursen commented 6 years ago

[x] Add position type errors and test
[x] Add errors returned by buildResults and test
[x] Remove hard coding of poll type

pietgeursen commented 6 years ago

Hey @mixmix, Keen to get your thoughts on the shape of the returned poll observable. It's not the same shape as the decorated poll object that poll.async.get returns which I'm not very happy about. What do you reckon? Is it good enough if we just document it well?

mixmix commented 6 years ago

looking at this now

REVIEW

This is a bit of a rambly review sorry. I enjoyed reading over it and it had me thinking about what good pull / mutant patterns are from readability and performance might be. Forgive me for thinking out loud, the quantity I wrote is about me not being sure.

I tried to flag the important points with red : :fire:

looks like you covered most things well here
- haven't compared the api of this pollDoc closely to the other async one :arrow_right: can we discuss what was different
i think pull-notify might be a little bit on the side of overkill here. I'm nervous about the push aspect it adds.

pietgeursen commented 6 years ago

@mixmix Ok, I'm pretty happy with this. And also ready to just do something different. I've been on this one PR for ages so keen to get momentum back.

Do you want to cast your eye over it?

pull notify is gone
sorting is now done only once on all the existing records in the db. And from there they are sorted again as new ones come in on the live stream.
I've changed the shape of the returned pollDoc so that is matches the one that you get with async.get. Although the actions are missing off this one I just realised.