-
Notifications
You must be signed in to change notification settings - Fork 4
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
Feat: Pubsub Datachannel #16
Conversation
@luongngocminh is attempting to deploy a commit to the 8xff Team on Vercel. A member of the Team first needs to authorize it. |
packages/sdk-core/src/data.ts
Outdated
@@ -116,6 +121,109 @@ export class Datachannel extends EventEmitter { | |||
} | |||
} | |||
|
|||
public async requestStartPublishChannel(req: { |
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 leave all of details to msg_channel. I think in data.ts we only implement abstract like: requestMsgChannel
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.
Isn't data.ts
purpose is to communicate to the server through implemented apis? msg_channel
feature is depended on data.ts
, so shouldn't the abstraction be on the msg_channel
side? Developer should be able call data.ts
in anyway they prefered without any abstraction.
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.
@luongngocminh No, data.ts is an abstraction for contacting servers. We don't need to update data.ts each time the protocol of Sender, Receiver, or MessageChannel ... changes.
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
) { | ||
const ctx = useContext(Atm0sMediaContext); | ||
const [channel, setChannel] = useState<RoomMessageChannel | null>(null); | ||
useEffect(() => { |
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 we need a function same with ctx.getOrCreatePublisher. Without it we will leak datachannel each time useEffect re-run
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.
instead of keeping a registry of msg channels in the react lib like the publishers, I've updated the core sdk to natively track the channels creation
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.
@luongngocminh It look good. Can you create same mechanism with publishers after this PR is merged?
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.
@giangndm do you want to move the publisher class into the core sdk? Because now it's only available in the react hooks side
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 will merge it now
Pubsub Datachannel implementation following this pr: 8xFF/atm0s-media-server#398![image](https://private-user-images.githubusercontent.com/9526028/354812284-5c29b396-7961-49ea-b784-249ee61873bb.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjU5MzgyMjgsIm5iZiI6MTcyNTkzNzkyOCwicGF0aCI6Ii85NTI2MDI4LzM1NDgxMjI4NC01YzI5YjM5Ni03OTYxLTQ5ZWEtYjc4NC0yNDllZTYxODczYmIucG5nP1gtQW16LUFsZ29yaXRobT1BV1M0LUhNQUMtU0hBMjU2JlgtQW16LUNyZWRlbnRpYWw9QUtJQVZDT0RZTFNBNTNQUUs0WkElMkYyMDI0MDkxMCUyRnVzLWVhc3QtMSUyRnMzJTJGYXdzNF9yZXF1ZXN0JlgtQW16LURhdGU9MjAyNDA5MTBUMDMxMjA4WiZYLUFtei1FeHBpcmVzPTMwMCZYLUFtei1TaWduYXR1cmU9YjI1MWY3MTc3MzU5ZTM5MGVjMGQyNzE4YWIwMzkzZjdmYmRkMDQ1NDlkYjExZTNlNmI3NWQ5YTlkN2M4MWJiOCZYLUFtei1TaWduZWRIZWFkZXJzPWhvc3QmYWN0b3JfaWQ9MCZrZXlfaWQ9MCZyZXBvX2lkPTAifQ.TB9e6M55viZMoV5E7IV3a0goUeQCl_RQyIF7RL8YloU)
User can test the msg channel feature in the 2 of the
echo_fast
examples: