Skip to content

[go_router] Don't clear listeners when logging is disabled #2533

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

Merged
merged 3 commits into from
Sep 8, 2022

Conversation

johnpryan
Copy link
Contributor

@johnpryan johnpryan commented Aug 30, 2022

go_router should not clear listeners on the root logger. See also dart-archive/logging#118

cc: @devoncarew

Fixes flutter/flutter#106653

go_router should not clear listeners on the root logger
Fixes #106653
Copy link
Contributor

@chunhtai chunhtai left a comment

Choose a reason for hiding this comment

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

LGTM

@DartAndrik
Copy link

You guys are incredible. Thank you. Just one more thing.
Could you add @visibleForTesting for setLogging or make it private and used for GoRouter instantiation only? As setLogging can be called after the GoRouter instance already configured like in the example for this package:

 final _router = GoRouter(
    debugLogDiagnostics: true,
    routes: [
      GoRoute(
        path: '/',
        builder: (context, state) => const Page1Screen(),
      ),
      GoRoute(
        path: '/page2',
        builder: (context, state) => const Page2Screen(),
      ),
    ],
  );

It may be confusing why setLogging(false) didn't do such switching off package logging.

@johnpryan johnpryan changed the title Don't clear listeners when logging is disabled [go_router] Don't clear listeners when logging is disabled Sep 1, 2022
Copy link
Contributor

@chunhtai chunhtai left a comment

Choose a reason for hiding this comment

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

LGTM

@chunhtai chunhtai added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 1, 2022
@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Sep 2, 2022
@auto-submit
Copy link
Contributor

auto-submit bot commented Sep 2, 2022

auto label is removed for flutter/packages, pr: 2533, Failed to merge pr#: 2533 with OperationException(linkException: null, graphqlErrors: [GraphQLError(message: Base branch was modified. Review and try the merge again., locations: [ErrorLocation(line: 2, column: 3)], path: [mergePullRequest], extensions: null)]).

@chunhtai
Copy link
Contributor

chunhtai commented Sep 8, 2022

@johnpryan It looks like there is merge conflict

@johnpryan johnpryan added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 8, 2022
@auto-submit auto-submit bot merged commit ba3a520 into flutter:main Sep 8, 2022
@johnpryan johnpryan deleted the fix-logging branch September 30, 2022 20:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App p: go_router
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[go_router] Passing debugLogDiagnostics: false clears my log listeners
3 participants