Add @firebase/logger as dependency to app-types#5144
Add @firebase/logger as dependency to app-types#5144Feiyang1 merged 4 commits intofirebase:masterfrom
Conversation
DennisSimon
requested review from
Feiyang1,
hiranya911 and
hsubox76
as code owners
🦋 Changeset detectedLatest commit: e7f7cc9 The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 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 |
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
|
@googlebot I signed it! |
| "devDependencies": { | ||
| "typescript": "4.2.2" | ||
| "typescript": "4.2.2", | ||
| "@firebase/logger": "0.x" |
There was a problem hiding this comment.
Thanks for the contribution! It should be a peerDependencies instead. Can you please make the change?
There was a problem hiding this comment.
Hi @Feiyang1 , thanks for the comment!
I have one question regarding this change:
So from my understanding, marking this as a peerDependency tells node that it should look for the module somewhere else in the dependency-tree, as it should already be installed.
Yet in my case @firebase/logger is not included anywhere in the dependency-tree of firebase-admin and will therefore not be found even when adding it as a peer dependency.
In this case, would i still add @firebase/logger as a peerDependency here and should i then add @firebase/logger as a dependency in firebase-admin?
There was a problem hiding this comment.
@firebase/logger is a dependency of @firebase/database, so it should be installed.
However, yarn pnp mode has stricter rules that prevent packages to require dependencies that they don't list explicitly, so I think that's why you are seeing the error.
Is there an easy way we can test it locally?
There was a problem hiding this comment.
So i tested it by adding the following entry to my .yarnrc.yaml file:
packageExtensions:
"@firebase/app-types@*":
peerDependencies:
"@firebase/logger": "0.x"After running yarn install, i got the following message:
➤ YN0002: │ @firebase/database-types@npm:0.7.2 doesn't provide @firebase/logger (pbcca2), requested by @firebase/app-types
So it looks like yarn is searching for peer-dependencies of @firebase/app-types only one level up?
After extending my .yarnrc.yaml and declaring @firebase/logger as a dependency of @firebase/database-types, the peer dependency is resolved and the compilation works.
packageExtensions:
"@firebase/database-types@*":
dependencies:
"@firebase/logger": "0.x"
"@firebase/app-types@*":
peerDependencies:
"@firebase/logger": "0.x"I'll look into it more tomorrow but at this point i think it is also possible that this might also be a bug of yarn.
I will change the dependency type to a peerDependency.
There was a problem hiding this comment.
So it looks like yarn is searching for peer-dependencies of @firebase/app-types only one level up?
It kinda makes sense. The peerDependencies of package A should be provided by packages that directly depend on A.
So I think we may need to make @firebase/logger a dependency of @firebase/app-types, and let's use 0.2.6 instead of 0.x.
There was a problem hiding this comment.
Looking at the code of yarn here it seems like it tries to follow the tree up.
I think that would also make sense for projects like firebase that exist (for the most of it) in a monorepo.
Well, i can check with yarn if this is expected behaviour or if its a bug but for now i agree that we can add @firebase/logger as a dependency of @firebase/app-types.
There was a problem hiding this comment.
SG. Can you please add a changeset to this PR via the link in #5144 (comment) ?
There was a problem hiding this comment.
Sure thing, added the changeset.
DennisSimon
changed the title
|
Hi @Feiyang1, quick ping as a reminder that the changeset is added |
2 participants
Hey everyone,
i ran into an issue with the
app-typesmodule where the typescript compilation of a module that definedfirebase-adminas a dependency failed with the following error:The project uses yarn2 with a 'strict' PnP mode.
I have created a small codesandbox that demonstrates this issue: https://codesandbox.io/s/gallant-satoshi-fycm1
Run
yarn compileto see the issue in action.Looking forward to your review!