Allow localtrack passing on BarVisualizer #1222
Conversation
🦋 Changeset detectedLatest commit: 0283eb5 The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
size-limit report 📦
|
…ents-js into lukas/allow-track-passing
lukasIO
changed the title
| // Warning: (ae-forgotten-export) The symbol "ReceivedMessageWithType" needs to be exported by the entry point index.d.ts | ||
| // | ||
| // @public (undocumented) | ||
| export interface ReceivedChatMessage extends ChatMessage { | ||
| // (undocumented) | ||
| attributes?: Record<string, string>; | ||
| // (undocumented) | ||
| export type ReceivedAgentTranscriptionMessage = ReceivedMessageWithType<'agentTranscript', { | ||
| message: string; | ||
| }>; | ||
|
|
||
| // @public (undocumented) | ||
| export type ReceivedChatMessage = ReceivedMessageWithType<'chatMessage', ChatMessage & { | ||
| from?: Participant; | ||
| } | ||
| attributes?: Record<string, string>; | ||
| }>; | ||
|
|
There was a problem hiding this comment.
thought: It looks like some changes from #1207 didn;t get included in this file - sorry about that, my bad 😞
However, I am a bit suprised that the ci step didn't check this? Maybe this is something to make a ticket on to look into, since it seems like this file isn't being checked like the associated components-reactone is.
There was a problem hiding this comment.
we don't check the API compat on CI for the core module as that's treated as an internal package.
Don't have a strong opinion on whether or not to change that.
There was a problem hiding this comment.
Ah, I see what you are saying, I guess that makes a certain amount of sense. Thinking about this in another way then - is there a good reason to even have the api-extractor tool run on the core package and generate this artifact if it won't be consumed externally?
There was a problem hiding this comment.
yeah, good point, both options sound fine to me!
| /** @deprecated use `track` field instead */ | ||
| trackRef?: TrackReferenceOrPlaceholder; | ||
| track?: TrackReferenceOrPlaceholder | LocalAudioTrack | RemoteAudioTrack; |
There was a problem hiding this comment.
suggestion: Other components currently seem to favor the trackRef prop name rather than track. I think I understand why you did it here (as a stepping stone to migrating away from TrackReference) but this means this component has two similar props which could be used at the same time, resulting in a less clear end interface.
What about instead, making BarVisualizerProps actually have two versions in a discriminated union, one that is the pre-existing version with trackRef (plus adding the deprecation), and the other getting rid of trackRef and adding track instead? The nice thing about doing it that way is the fields are mutually exclusive.
There was a problem hiding this comment.
It's somewhat cleaner that way, true. But the downside of that is that the props wouldn't be as easy to inspect anymore, right?
I thought deprecating one of them is pretty clear?
There was a problem hiding this comment.
To be clear I don't really have a strong opinion on this, it just struck me reading this code that it would be nice if they were mutually exclusive since they really should never both be set and then once you've migrated over to the new track prop, it is as if the old trackRef prop no longer exists .
By "props wouldn't be as easy to inspect anymore" if you mean that the component props wouldn't point to a singular interface on a "go to definition" type operation and instead would be an expression of some sort - yes, that is a good point. I guess there are pros and cons to both approaches, so do whatever you think makes sense.
BarVisualizer should also accept audio tracks directly