-
Notifications
You must be signed in to change notification settings - Fork 71
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
revert upgrade to protobuf-es 2 #368
Conversation
|
} from './proto/audio_frame_pb.js'; | ||
|
||
export class AudioSource { | ||
/** @internal */ | ||
info: AudioSourceInfo; | ||
info?: AudioSourceInfo; |
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.
in what situation would this be missing?
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.
unsure, but in general i prefer to use ?
over !
if the former works. could be swayed to replace it with an exclamation point later in the file
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.
if we expect an AudioSource
to always have a AudioSourceInfo
(which I think we do as it's required in the FFI protocol), I think we should reflect that on the user facing API.
import type { Track } from './track.js'; | ||
|
||
export class AudioStream implements AsyncIterableIterator<AudioFrame> { | ||
/** @internal */ | ||
info: AudioStreamInfo; | ||
info?: AudioStreamInfo; |
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.
same question as above
@@ -61,40 +58,40 @@ import type { ChatMessage } from './types.js'; | |||
|
|||
export abstract class Participant { | |||
/** @internal */ | |||
info: ParticipantInfo; | |||
info?: ParticipantInfo; |
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.
same question
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.
lgtm!
packages/livekit-rtc/package.json
Outdated
@@ -46,17 +46,17 @@ | |||
} | |||
}, | |||
"dependencies": { | |||
"@bufbuild/protobuf": "^2.2.0", | |||
"@bufbuild/protobuf": "^1.4.2", |
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.
"@bufbuild/protobuf": "^1.4.2", | |
"@bufbuild/protobuf": "^1.10.0", |
packages/livekit-rtc/package.json
Outdated
"@napi-rs/cli": "^2.18.0", | ||
"@types/node": "^20.9.2", | ||
"prettier": "^3.0.3", | ||
"tsup": "^8.3.5", | ||
"typescript": "^5.2.2" | ||
"typescript": "^5.2.2", | ||
"@bufbuild/protoc-gen-es": "^1.4.2" |
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.
"@bufbuild/protoc-gen-es": "^1.4.2" | |
"@bufbuild/protoc-gen-es": "^1.10.0" |
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.
🥳
This reverts commit f204721, which introduces various bugs with version mismatches between protobuf-es 1.x and 2.x. see livekit/agents-js#232
would appreciate as many eyes on this as possible, to:
!
(i think it's safe to use when necessary, due to optional/required, but worth checking)still not done with room.ts because it is big and messy