Add usePresence for solid primitives#549
Conversation
|
Thanks for the contribution! FYI, @neftaly suggested also exposing start/stop; I'm doing that in #544. He also suggested being able to change the update interval. Right now, you can do that by starting and stopping, but the React hook does not expose that. I'll take a look at incorporating that, and it would probably make sense here as well. |
|
Hey, Thanks for the feedback! I'll add those parameters asap. |
|
hey @msakrejda, quick question: when you mention the update interval you're talking about the heartbeatMs attribute in the Presence class, right? Or am I missing something? |
|
@matfire I think so. I want to be able to have a peer declare itself as connected, but be idle or in low-power mode. Eg, a phone trying to optimize battery/radio use, or clients optimizing sync server bandwith. |
|
then it should already be implemented, I think. You can pass in both the heartbeat rate & the peer ttl in the initial hook definition and in the exposed start method. is there something more that's needed than that ? |
|
Can I trivially switch between fast and slow heartbeat rate, while the app is running? If this can be done just by changing a prop or arg then it should be good. I am very unfamiliar with these codebases :) |
|
Aside, It may also be useful to have a way to force a heartbeat. But I don't need this feature atm. |
|
mmh, I don't think the Presence class exposes just changing the heartbeat, so you would still need to stop & start the reference with a different heartbeat (doable in the current state of the pr). Or I'd need to modify the Presence class directly by adding a setter for the heartbeat (and handle restarting the timer for it etc); this would also imply updating the react usePresence hook to keep feature parity |
|
I think it's okay to require starting / stopping Presence to adjust heartbeat intervals. Starting / stopping is pretty lightweight and can be done with no visible impact to the Presence state (I think). I don't think it's worth complicating the API to support adjusting heartbeats while presence is running. Support for forcing heartbeats also seems pretty niche... |
Sorry I missed that: yeah, just the heartbeat interval. |
then we're good to go 👍 |
|
A test for the switching scenario would be really nice :) I'll make an issue |
|
Got it! Since there's a separate issue, should the tests be in a separate pr or (at least for solid) is it fine to add it in here ? |
|
It's fine to add them to this PR. i don't know if there should be separate tests for solid and react, if so please add a note about missing react tests to the issue. |
|
Should we test something like <Component heartbeatMs= |
heartbeat rate from props
|
added a few more tests to also set a custom heartbeat on "mount" |
| const [presence] = createSignal( | ||
| new Presence<State>({ handle, userId, deviceId }) | ||
| ) |
There was a problem hiding this comment.
i don't think we get anything from this being a signal, as it isn't tracking and doesn't change.
| ) | ||
| } | ||
|
|
||
| onMount(() => { |
There was a problem hiding this comment.
onMount is a non-tracking function that runs in an effect, i think for our purposes the behaviour will be essentially the same if we move the body of it out to the top level of usePresence
|
this looks good, i left a couple comments |
|
thanks for the feedback @chee, fixed it; let me know if this is what you meant |
|
Does this need updates for the PeerStateView changes in #555? I thought it might at first, but now I think it's fine, though I'm not 100% sure. Could you rebase on top of those changes? |
|
updated the fork, tests still seem to pass |
| }: UsePresenceConfig<State>): UsePresenceResult<State> { | ||
| const [localState, setLocalState] = createStore<State>(initialState) | ||
| const [peerStates, setPeerStates] = createStore(new PeerStateView<State>({})) | ||
| const presence = new Presence<State>({ handle, userId, deviceId }) |
There was a problem hiding this comment.
So after #555, the Presence constructor no longer takes userId or deviceId arguments. Handling aggregation by user, device, or other higher-level concept is done at read time.
|
Any reason not to merge this? |
4 participants
Uh oh!
There was an error while loading. Please reload this page.