Skip to content

Remove unnecessary lcov-result-merger dep from @firebase/messaging #547

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

Jump to bottom
Merged
merged 1 commit into from
Mar 6, 2018
Merged

Remove unnecessary lcov-result-merger dep from @firebase/messaging #547

merged 1 commit into from
Mar 6, 2018

Conversation

zevdg
Copy link
Contributor

@zevdg zevdg commented Mar 4, 2018

tentatively fixes #546

I'm not sure that making grpc an optional dependency is the best way to fix this, but I figured I'd create this PR as a starting point.

gauntface
gauntface approved these changes Mar 5, 2018
View reviewed changes
Copy link

@gauntface gauntface left a comment

Choose a reason for hiding this comment

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

LGTM - but would like @jshcrowthe to double check if lcov merger is needed elsewhere. Would have expected it to be a top level dep - not something in any of the modules.

packages/messaging/package.json
@@ -18,7 +18,6 @@
"dependencies": {
"@firebase/messaging-types": "0.1.2",
"@firebase/util": "0.1.10",
"lcov-result-merger": "^2.0.0",
Copy link

Choose a reason for hiding this comment

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

@jshcrowthe not sure why this is in dependencies but might be needed in devdeps?

Copy link
Contributor

@jshcrowthe jshcrowthe Mar 5, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

This actually isn't needed at all in this package. We use it at a higher level. It seems it snuck in, in #482

@jshcrowthe
Copy link
Contributor

Per comments in #546, I don't think the changes related to grpc are something we want to keep. That said, the change related to the lcov-result-merger package is something we'll need to do and I'm happy to get you contributor cred 😄 !

Can we refactor this PR to remove only that dep?

@zevdg
Copy link
Contributor Author

zevdg commented Mar 5, 2018

there you go.

@jshcrowthe jshcrowthe changed the title removes dependencies that cause problems for yarn install --flat Remove unnecessary lcov-result-merger dep from @firebase/messaging Mar 6, 2018
jshcrowthe
jshcrowthe approved these changes Mar 6, 2018
View reviewed changes
Copy link
Contributor

@jshcrowthe jshcrowthe left a comment

Choose a reason for hiding this comment

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

Awesome thanks @zevdg!

@jshcrowthe jshcrowthe merged commit 2b20a14 into firebase:master Mar 6, 2018
@zevdg zevdg deleted the flat branch March 6, 2018 19:39
@firebase firebase locked and limited conversation to collaborators Oct 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@gauntface gauntface gauntface approved these changes

@jshcrowthe jshcrowthe jshcrowthe approved these changes

@mikelehen mikelehen Awaiting requested review from mikelehen

@schmidt-sebastian schmidt-sebastian Awaiting requested review from schmidt-sebastian

@wilhuff wilhuff Awaiting requested review from wilhuff

Assignees
No one assigned
Labels
needs-triage
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

firebase doesn't play nicely with yarn install --flat
4 participants
@zevdg @jshcrowthe @gauntface @google-oss-bot