-
Notifications
You must be signed in to change notification settings - Fork 1.2k
fix: client import & server imports #851
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
base: main
Are you sure you want to change the base?
Conversation
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! 👍
@cliffhall could you approve it as well? Seems like merging is blocked |
Since client/index is about to be having its own export, probably good to do it with the server? Also, index.ts is not a barrel file in this SDK, so you'll get a limited amount of types/classes exported by it. Have suggested going for a single point of exports in #716, an introduction of a barrel index file can be done without it being a breaking change for both clients and servers. |
Also, in the title for this PR, what does RN refer to? This is a change to package.json |
This makes sense. @jatinsandilya would you care to add that?
@KKonstantinov agreed, but until we get a single point of exports (a good Idea I heartily support), this PR fixes a current problem in the field, and I think we should move ahead with it when we have the servers/index change added. Do you foresee any problems with refactoring toward index.ts being single barrel file subsequently? |
Ah it was React Native, since the original issue was about RN.
|
Closes issue #511
Motivation and Context
Client imports were broken
How Has This Been Tested?
This is a trivial Metro bundler. Metro seems to have partial support for the
exports
keyword. Adding a couple of explicit ./client / ./client/index.js entries is the easiest cross-tool fix.Breaking Changes
Types of changes
Checklist
Additional context