-
Notifications
You must be signed in to change notification settings - Fork 130
Change useAgent to return undefined for nullable TrackReference values
#1224
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
Conversation
…ce values This is being done to make them fit a bit better into the existing `trackRef` prop on a few components, which right now accepts `TrackReference | undefined`. I think given the history of how things evolved with `TrackReference`s it makes sense to do this, but I think it's a rather unfortunate change to make since null and undefined semantically tend to mean different things ([more info](https://kettanaito.com/blog/the-difference-between-null-and-undefined), [other interesting read](https://medium.com/@stephenthecurt/a-brief-history-of-null-and-undefined-in-javascript-c283caab662e)) and this case to me is semantically much more of a "null" (explicit absence of a value) than an "undefined" (value was never set).
🦋 Changeset detectedLatest commit: 33c710c 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 |
undefined for nullable TrackReference valuesundefined for nullable TrackReference values
size-limit report 📦
|
| // Clear inner values if no longer connected | ||
| cameraTrack: null, | ||
| microphoneTrack: null, | ||
| cameraTrack: undefined, | ||
| microphoneTrack: undefined, | ||
| }; |
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.
Just wanted to call out that I kept these key: undefined type entries here so that destructuring would continue to work coming out of useAgent - ie, const { cameraTrack, microphoneTrack } = useAgent(session). Otherwise this pattern would break down because cameraTrack + microphoneTrack wouldn't always be in the return value.
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 agree we can/should make more deliberate choices about undefined vs null for future versions. Thanks for addressing and treating it as a compatibility fix
This is being done to make them fit a bit better into the existing
trackRefprop on a few components, which right now acceptsTrackReference | undefined.I think given the history of how things evolved with
TrackReferences it makes sense to do this, but I think it's a rather unfortunate change to make sincenullandundefinedsemantically tend to mean different things in javascript (more info, other interesting read) and this case to me is semantically much more of a "null" (explicit absence of a value) than an "undefined" (value was never set).Maybe it's something to litigate further as part of the next major version, however.