Skip to content

Add @firebase/logger as dependency to app-types #5144

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Jul 23, 2021
Merged

Add @firebase/logger as dependency to app-types #5144

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
04abecf
Add @firebase/logger as devDependency to app-types
DennisSimon Jul 12, 2021
41d8b2d
Change @firebase/logger dependency type to peerDependency
DennisSimon Jul 13, 2021
d51960a
Pin @firebase/logger version and add as a dependency
DennisSimon Jul 13, 2021
e7f7cc9
Add changeset
DennisSimon Jul 14, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion packages/app-types/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
"url": "https://github.com/firebase/firebase-js-sdk/issues"
},
"devDependencies": {
"typescript": "4.2.2"
"typescript": "4.2.2",
"@firebase/logger": "0.x"
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the contribution! It should be a peerDependencies instead. Can you please make the change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

@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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

@Feiyang1 Feiyang1 Jul 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

SG. Can you please add a changeset to this PR via the link in #5144 (comment) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure thing, added the changeset.

}
}