-
Notifications
You must be signed in to change notification settings - Fork 897
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
Share code with Admin SDK properly #5537
Conversation
🦋 Changeset detectedLatest commit: 7ad9ecd The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Changeset File Check ✅
|
Binary Size ReportAffected SDKs
Test Logs |
Size Analysis Report |
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.
Looks mostly good (and a lot cleaner). A couple of nits and a question.
} from './api/test_access'; | ||
/* eslint-enable camelcase */ | ||
export * from './api.standalone'; | ||
export { getDatabase } from './api/Database'; |
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.
Thanks!
setWebSocketImpl(Client); | ||
|
||
// This entry point should only be consumed by Admin SDK | ||
export * from './api.standalone'; |
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.
Missing empty line.
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.
Thanks. LGTM 👍
Does this need any kind of tests?
It might be worth adding a test to make sure that the build for the standalone entry point doesn't pull in |
Share database code with Admin SDK via a dedicated entry point which doesn't depend on
@firebase/app
.Admin SDK is currently broken with v9
@firebase/database-compat
. After this change is merged, Admin needs to change imports from@firebase/database-compat
to@firebase/database-compat/standalone
.