-
Notifications
You must be signed in to change notification settings - Fork 517
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
fix(presence-tracker): rename TinyliciousMember.userName to TinyliciousMember.name #21111
Conversation
@@ -6,7 +6,8 @@ | |||
import { Signaler } from "@fluid-experimental/data-objects"; | |||
import { TypedEventEmitter } from "@fluid-internal/client-utils"; | |||
import { IEvent } from "@fluidframework/core-interfaces"; | |||
import { IMember, IServiceAudience } from "fluid-framework"; | |||
import type { ITinyliciousAudience } from "@fluidframework/tinylicious-client/internal"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should not use /internal in most examples if we can help it. I know that we do sometimes, but largely these are supposed to be demonstrate what customers can do. /beta (and /alpha when it is available) are okay.
Also, client code should be service agnostic when possible, except for the actual connection establishment.
So, should not use tinylicious-client here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think service-audience data might be an exception to service agnosticism, since we market it as a service-specific feature to customers (https://fluidframework.com/docs/build/audience/#service-specific-audience-data)
I'll look into ways to not use /internal APIs. However, I'm not sure these examples are to demonstrate what external customers should/could do. I remember asking Matt @ChumpChief so maybe he can help here, but I think these serve as mostly for testing(?) I could be wrong though. /internal is used a lot here so it's confusing, I think FluidExamples repo is more geared for external customer usage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct that the audience is not service-agnostic (though it would be nice if we could make it so).
I think it could be reasonable to use IAzureAudience here instead, as we typically talk about Tinylicious as being the "Azure local service" and support customers writing code to run equally against Tinylicious/Azure. Maybe ITinyliciousAudience should go away even in favor of IAzureAudience (at least if we're not going to make it public)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure offhand why IUser is public, but I think in general AzureMember is what should show up on the public interface (whereas AzureUser is more internal-facing). I'm a little fuzzy on this area though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
We recently renamed the AzureMember.userName to AzureMember.name to establish uniform naming across odsp-client and azure-client.
This change needs to be reflected in the presence tracker example since it uses audience to display focus status and mouse position of all connected clients. The example right now is essentially just a blank screen.