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

Allow returning user IDs from useParticipantIds #7

Open
rileyjshaw opened this issue Jan 4, 2023 · 6 comments
Open

Allow returning user IDs from useParticipantIds #7

rileyjshaw opened this issue Jan 4, 2023 · 6 comments
Assignees
Labels
enhancement New feature or request

Comments

@rileyjshaw
Copy link
Contributor

rileyjshaw commented Jan 4, 2023

Hi again! Since my last feature request sparked some good discussion, I’m adding another idea.

Feature request

Add an idType param to useParticipantIds to allow returning user IDs, instead of the default behaviour which returns session IDs.

useParticipantIds({ idType: 'user' });

Why you need this

My application stores user data in a map keyed by participant user IDs. It would be really helpful to get a list of user IDs directly from this hook, rather than needing to store them in a separate callState object as shown here.

Alternatives you've considered

This could go into its own hook called useUserIds or useParticipantUserIds, but that feels a bit heavy vs. the above proposal.

Additional context

I can make a quick draft PR if this sounds useful.

@rileyjshaw rileyjshaw added the enhancement New feature or request label Jan 4, 2023
rileyjshaw added a commit to rileyjshaw/daily-react that referenced this issue Jan 4, 2023
@Regaddi Regaddi self-assigned this Jan 5, 2023
@Regaddi
Copy link
Contributor

Regaddi commented Feb 27, 2023

@rileyjshaw I have been noodling on this a little longer now and wanted to provide an update:

Rather than only changing useParticipantIds to return either a list of session_ids or user_ids, I'd like to work on a more general solution for better user_id support.
The idea is to add a prefferedParticipantIdType prop to DailyProvider. Possible values are "session_id" (default) and "user_id".
Changing this prop will change the overall behavior of components and hooks to refer to participantIds rather than sessionIds. When switching to "user_id", useParticipantIds as an example will always return a list of user_ids, instead of session_ids.
In general there should be only one type of id developers should have to deal with.

This will be a bigger change and it will take a bit to get it done and wrapped up, but I think it's a more concise path forward compared to changing only a few hooks to allow for better support of user_id.

Let me know in case you think this doesn't sound like the right approach to the underlying issue, or if you have additional thoughts or context on the usage of user_ids.

Cheers,
Christian

@rileyjshaw
Copy link
Contributor Author

@Regaddi thanks for following up!

It’s an interesting idea, but it would not work for my use case. Needing to standardize on either user_id or session_id would be a big undertaking in my codebase, since currently, many properties only work with session_id. For example, useActiveSpeakerId, useVideoTrack, useAudioTrack, setSubscribedTracks, etc. I imagine that some of these could be changed to use user_id based on the preferredParticipantId prop, but could they all change? Based on my knowledge of the underlying daily-js library, some of these functions require session_id.

I also think the preferredParticipantIdType prop might cause some confusion in existing components. For example, would DailyAudioTrack’s sessionId prop become id, and expect either user_id or session_id depending on the preferredParticipantIdType prop?

Sorry for the push-back, just sharing my honest initial reaction. I think I’d end up needing to frequently toggle preferredParticipantId, which would likely cause issues in functions and components that expect one or the other.

@rileyjshaw
Copy link
Contributor Author

…and for what it’s worth, I am a fan of eventually being able to use user_id in more places. Managing two IDs for each participant (and additional IDs for screenshares, custom audio, etc) has required a lot of code for ID management. I’ll be very happy if I can delete all of my session_id handling someday. But doing so via preferredParticipantIdType feels like it might introduce a fragile transition period.

@Regaddi
Copy link
Contributor

Regaddi commented Feb 27, 2023

Thank you for the thoughtful reply!

We wouldn't change underlying daily-js APIs to accept user_id instead of session_id. updateParticipant(s) is a good example for this.

In adding support for user_id, either as an optional selection path or as a replacement for session_id in hooks and components, I was sort of wondering why one would want to maintain 2 separate ids per participant in a call. It seems like unnecessary overhead work having to convert user_id to session_id or back over and over again for all rendering parts.

The proposal to add a top-level control via DailyProvider seemed initially like a convenient way to flip the behavior between session_id and user_id, but I agree that this could lead to more confusion, especially in bigger codebases, and introduce unclarity when working with components and hooks. Additionally we might have to change or extend some of our component and hook APIs to avoid ambiguity, e.g. <DailyVideo sessionId={...} />, as you already pointed out.

Perhaps it would require some more work to achieve the desired granularity in working with different parts of our API. useActiveSpeakerId is a good example, because it's based on the active-speaker-change event, which always refers to a participant's session_id. Changing the behavior of this hook to return the active participant's user_id instead is probably non-trivial.

Anyway I don't think that introducing an API change in useParticipantIds only is a good idea, before thinking through the bigger implication of allowing user_id to be a first-class selector for our components and hooks.

I think my general proposal to achieve this would look something like this:

  • Rename occurrences of sessionId as props or hook arguments to participantId
  • Allow developers to switch between session_id and user_id as the primary selector for components and hooks with a participantId prop/argument (either by adding an optional prop/argument to each component/hook and/or by adding a top-level prop to change the default selection behavior)

The actual change really depends on the migration path. And I definitely agree that we wouldn't want to introduce a fragile transition period.
I'll keep this on my inner back-burner a bit. For the time being some kind of custom to daily-js id management is required anyway.

@rileyjshaw
Copy link
Contributor Author

Thanks @Regaddi, that makes sense. I agree that it’s probably best to keep this on the backburner. My main issue is that I need to manage multiple sets of IDs, and that’s not specific to the daily-react library.

Further context

My app manages most user state outside of Daily. Users have unique IDs, and exist in a big user map:

const users = {
  "1234-abcd-5678-efgh": {
    name: "Wendy",
    avatar: 7,
    // ...
    sessionId: "lmno-pqrs-tuvw",
  },
  "2345-bcde-6789-fghi": {
    name: "George",
    avatar: 3,
    // ...
    sessionId: "wxyz-abcd-efgh",
  },
  ...
}

When a user joins the Daily call, I set their user_id in the token to match the common UID. Then once they sign in with Daily, I add their sessionId to the user map (shown above).

This works, but it’s cumbersome for situations where I need to see combine user state with Daily state. For instance, if I want a list of Daily screenshares and their associated user names, I need to do something like this:

// ...
const sessionIds = useParticipantIds({ filter: 'screen' });

sessionIds.map(sessionId => {
  return {
    name: users.find(user => user.sessionId === sessionId.slice(-6)).name,
    sessionId,
  };
);

If I could use user_id directly with Daily’s utils (or set session_id to user_id in the initial auth token), it would mean I wouldn’t need to maintain a map of sessionId => userId pairs. So I think this issue is more about the mixed use of user_id and session_id, rather than anything specifically about useParticipantIds or daily-react.

Thanks for listening! 😅 and please let me know if you think I’m overlooking something.

@troutowicz
Copy link

FWIW, the solution that works for us is grabbing the userId right before we need it.

const userId = useParticipantProperty(sessionId, 'user_id')

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants