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

type fixes and some bugfixes #261

Merged
merged 27 commits into from
Jun 3, 2024
Merged

type fixes and some bugfixes #261

merged 27 commits into from
Jun 3, 2024

Conversation

mildsunrise
Copy link
Contributor

most of this is just fixing type errors (I'd like to start enforcing typechecking in CI, since it would have caught bugs such as this one)

but there's also a couple bugfixes.

I suggest reviewing commit by commit.

right now they are only present in LayerStats BUT ONLY
if returned from getStats(), not getActiveLayers().
a similar thing happens with EncodingStats.

this PR makes them always present.
(it's already checked for us in getMediaStats)
we do a dangerous pattern where we assume the type
of this.depacketizer depending on if it's present or
not. indicate this to the type checker.
the type system has some difficulty following
Symbol accesses
lib/ActiveSpeakerDetector.js Show resolved Hide resolved
lib/ActiveSpeakerMultiplexer.js Show resolved Hide resolved
lib/IncomingStream.js Show resolved Hide resolved
lib/IncomingStreamTrack.js Outdated Show resolved Hide resolved
lib/Transport.js Outdated Show resolved Hide resolved
lib/Transport.js Show resolved Hide resolved
lib/ActiveSpeakerMultiplexer.js Show resolved Hide resolved
lib/ActiveSpeakerMultiplexer.js Outdated Show resolved Hide resolved
lib/ActiveSpeakerMultiplexer.js Outdated Show resolved Hide resolved
lib/IncomingStreamTrack.js Outdated Show resolved Hide resolved
lib/Transport.js Show resolved Hide resolved
lib/Transport.js Outdated Show resolved Hide resolved
lib/Transport.js Outdated Show resolved Hide resolved
Copy link
Member

@murillo128 murillo128 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

lib/Transport.js Outdated Show resolved Hide resolved
@mildsunrise
Copy link
Contributor Author

only a few type annotations changed since last review, so I'll merge this in now

@mildsunrise mildsunrise merged commit 87fa32c into master Jun 3, 2024
2 checks passed
@mildsunrise mildsunrise deleted the dev/alba/track-type-fixes branch June 3, 2024 13:04
mildsunrise added a commit to medooze/video-codecs-node that referenced this pull request Jun 19, 2024
mildsunrise added a commit to medooze/audio-codecs-node that referenced this pull request Jun 19, 2024
mildsunrise added a commit to medooze/rtmp-server-node that referenced this pull request Jun 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants