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

remove @firebase/app and @firebase/app-types from peerDependencies #2082

Merged
merged 4 commits into from
Aug 16, 2019

Conversation

Feiyang1
Copy link
Member

@Feiyang1 Feiyang1 commented Aug 13, 2019

This is to solve the unmet peerDependency warning when @firebase/database is used in Admin SDK. firebase/firebase-admin-node#614

@firebase/app was intentionally removed from Admin SDK's dependency to solve a version conflict issue when it's used together with JS SDK. #1916

Though this PR is not the most correct way to solve the issue, it is quick and easy with little impact on user experience. The only user experience change is that user won't get unmet peer dependency warnings if @firebase/database is installed without installing @firebase/app.
It should be a rare case as we ask user to install firebase package in all our guides, which automatically includes @firebase/app.

The alternative solution is create a separate npm package for admin sdk consumption. It feels like an overkill and adds a lot of maintenance burden on us for little benefit.

Copy link
Contributor

@hiranya911 hiranya911 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks reasonable, but I had a couple of questions.

packages/database-types/package.json Outdated Show resolved Hide resolved
packages/database/package.json Show resolved Hide resolved
Copy link
Contributor

@hiranya911 hiranya911 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code change LGTM. Any idea why the CI has failed?

@hiranya911 hiranya911 assigned Feiyang1 and unassigned hiranya911 Aug 14, 2019
@Feiyang1
Copy link
Member Author

@schmidt-sebastian WDYT

Copy link
Contributor

@schmidt-sebastian schmidt-sebastian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Feiyang1 Feiyang1 merged commit 5b5afa2 into master Aug 16, 2019
@Feiyang1 Feiyang1 deleted the fei-database-deps branch August 16, 2019 17:15
@firebase firebase locked and limited conversation to collaborators Oct 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants