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

Re export agora-rtc-sdk-ng methods from the React SDK #146

Closed
EkaanshArora opened this issue Aug 28, 2023 · 10 comments
Closed

Re export agora-rtc-sdk-ng methods from the React SDK #146

EkaanshArora opened this issue Aug 28, 2023 · 10 comments
Assignees
Labels
feature request Feature Request

Comments

@EkaanshArora
Copy link
Contributor

What kind of problem does this feature solve?

Since we're positioning the React SDK as a standalone product. We should re-export all the exports from the web sdk. This is important as while using the React SDK the devs also need to access exports from Web like types, default export, enums etc.

What does the proposed API look like?

import AgoraRTC from 'agora-rtc-sdk-ng'

export default AgoraRTC;
export * from 'agora-rtc-sdk-ng';

Open to discuss if we should avoid a default export.

@EkaanshArora EkaanshArora added the feature request Feature Request label Aug 28, 2023
@guoxianzhe
Copy link
Collaborator

guoxianzhe commented Aug 29, 2023

It may be achieved though, but there are 3 potential problems:

  1. agora-rtc-sdk-ng size is very large, and the way of export * will have a relatively large impact on the agora-rtc-react size
  2. The independence between packages may be destroyed, and the documentation and unit testing cannot be well maintained
    such as .d.ts should include all agora-rtc-sdk-ng?
  3. This is not conducive to tree-shaking

@EkaanshArora
Copy link
Contributor Author

I agree with the problems you mentioned but we're targeting to release this is as a standalone SDK, we can't position this as a dependency on the Web SDK or a wrapper. So even though the concerns you mentioned are valid, we'll have to find the best course for release

  1. This is minor as if you're using this project you'd need the Web SDK installed and the bundle size would be identical in a real-life example.
  2. We need to figure out what we can do here.
    Yes, typings should contain all of Web SDK.
  3. We can try our best here, but tree-shaking would depend on the Web SDK team to implement for us to leverage.

@guoxianzhe
Copy link
Collaborator

guoxianzhe commented Aug 30, 2023

OK, please confirm is this you want?
for example ,

old:

import AgoraRTC from "agora-rtc-sdk-ng"

new:

import AgoraRTC from "agora-rtc-react"

package.json will be:
image

Are you sure our users can accept this? Looks like a big change so we need to be careful.

@EkaanshArora
Copy link
Contributor Author

Hey sorry for the wait on this. Discussing internally if we should mix both named exports and default export in the same package. This isn't considered a good practice as of late, but it follows more of the Web SDK pattern of exports so I'm torn. Which do you think is better?

import AgoraRTC, {useClientHook, ...} from "agora-rtc-react"

vs.

import {AgoraRTC, useClientHook, ...} from "agora-rtc-react"

@guoxianzhe
Copy link
Collaborator

I think we should use

import {AgoraRTC, useClientHook, ...} from "agora-rtc-react"

In case of export the default class AgoraRTCReact in the future, or we should consider (need websdk help here)

export class AgoraRTCReact extends AgoraRTC {}

usage:

import AgoraRTCReact from "agora-rtc-react"

And there is more thing, CDN is umd package , window is prefer to export AgoraRTCReact instead of AgoraRTC

@EkaanshArora
Copy link
Contributor Author

If we re-export Web SDK from React, another detail to discuss would be how we keep the Web SDK peer dependency up to date. We'll need to make sure that minor updates to the Web SDK don't require an update to the React SDK. New installs of the React SDK should get the latest releases of both (as long as Web SDK follows semantic versioning/backwards compatibility). This shouldn't be difficult to do, just something to keep in mind when we make this change.

@EkaanshArora
Copy link
Contributor Author

Argument against default exports: https://x.com/kentcdodds/status/1702339919438627290

@guoxianzhe
Copy link
Collaborator

guoxianzhe commented Oct 25, 2023

@EkaanshArora I did some for this feature, #163

import type { ILocalAudioTrack, ILocalTrack, ILocalVideoTrack } from "agora-rtc-react";
import AgoraRTC, {
  AgoraRTCScreenShareProvider,
  LocalAudioTrack,
  LocalVideoTrack,
  useJoin,
  usePublish,
  useTrackEvent,
} from "agora-rtc-react";

And window object (only in umd/iife)
企业微信截图_67e3433b-97b8-4566-83ed-f7b21c5211c6

Let me know if you have another ideas.

@EkaanshArora
Copy link
Contributor Author

Hey this looks good, let me give it a try and get back to you!

@guoxianzhe guoxianzhe self-assigned this Nov 18, 2023
@guoxianzhe
Copy link
Collaborator

completed in #163

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Feature Request
Projects
None yet
Development

No branches or pull requests

2 participants