-
Notifications
You must be signed in to change notification settings - Fork 52
Add cancelLogging method #118
Add cancelLogging method #118
Conversation
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
cc @devoncarew can you assign this to somebody in the ecosystem team to review? I meant to take a look but obviously haven't had a chance to, so unassigned myself. |
@DartAndrik - thanks for the PR! I have a few questions to help get my head around the problem (in the meantime though, in order for us to accept contributions, you'll need to sign the CLA):
Generally, we want to be very cautious about a major version rev. of a package like this - used by many other packages. We'll want to prefer a minor version rev. instead (this PR as written would be a minor version rev. - adding new API). I'm curious why go_router is calling clearListeners in the first place - why it feels like it needs to clear all logging listeners. And why there's a need in general to terminate a logger like that ( |
@devoncarew Yes, all 3 points are correct. And i saw that it can be fixed in Go_Router. In different ways: from The main idea is to add And yes. You are right. The other idea is to replace As for the major version - i totally agree. But i see that the |
Right, we do need to be careful here because we know the api is implemented. We could do a forward fix for package:build potentially... but it would leave out there some broken combinations of packages, and we also know that many users of build_runner are stuck on old versions because of being stuck on old analyzer versions. We could tell them to pin package:logging to an older version though if necessary. |
…n `logging ^1.0.0`
From talking to one of the go_router maintainers (@johnpryan), the use of My guess is that they'll remove the call from that package. That should solve the immediate issue. It still does leave open some questions about potential API design improvements to this package; things that would make unintentional interactions between package uses less likely. |
@DartAndrik - thanks for the contribution and for identifying the issue. Given the upcoming fix in flutter/packages#2533, I don't think we'll need to adjust the logging API in order to address the clearListeners() issue. For posterity, here are some existing issues and discussions about package:logging API improvements: |
To avoid cases like:
As the result i have my app logger deconfigured because of clearListeners method logic(stream controller for my logger was cleared too).
For examlpe package GoRouter uses setLogging method and i have to be carefull with the sequence of my app logger initialization and creation the instance of GoRouter. GoRouter uses GoRouter.setLogger first time just while constructing with clearListeners for GoRouter(debugLogDiagnostics: true). Even more, i have to check every time that GoRouter.setLogging have nor been used again latter by other developers in my app.
Version changed to 2.0.0 due to breaking changes. Class Logger is implemented in some packages (as in build_runner_core with BuildForInputLogger).