newrelic / newrelic-flutter-agent

New Relic agent SDK for Flutter hybrid mobile apps
Apache License 2.0
6 stars 12 forks source link

GoRouter observer causes exception #68

Closed mohadel92 closed 1 month ago

mohadel92 commented 9 months ago

while adding observers: <NavigatorObserver>[NewRelicNavigationObserver()],

to the go_router in my project :

an exception while navigation to one of my screens : package:flutter/src/widgets/navigator.dart': Failed assertion: line 2993 pos 18: '!navigator._debugLocked': is not true.

any help with this issue

═══════ Exception caught by widgets library ═══════════════════════════════════ The following NoSuchMethodError was thrown building InheritedGoRouter(goRouter: Instance of 'GoRouter'): Class 'RouteSettings' has no instance getter 'key'. Receiver: Instance of 'RouteSettings' Tried calling: key

The relevant error-causing widget was: MaterialApp MaterialApp:file:///Users/mohammedadelalisorour/Documents/My%20%20Work/Flutter/sevenSquare/the_laundry_hub/lib/app.dart:30:30

When the exception was thrown, this was the stack:

0 Object.noSuchMethod (dart:core-patch/object_patch.dart:38:5)

object_patch.dart:38

1 NewRelicNavigationObserver._addGoRouterBreadcrumb (package:newrelic_mobile/newrelic_navigation_observer.dart:83:30)

newrelic_navigation_observer.dart:83

2 NewRelicNavigationObserver.didPush (package:newrelic_mobile/newrelic_navigation_observer.dart:45:9)

newrelic_navigation_observer.dart:45

3 _NavigatorPushObservation.notify (package:flutter/src/widgets/navigator.dart:3338:14)

navigator.dart:3338

4 List.forEach (dart:core-patch/growable_array.dart:416:8)

growable_array.dart:416

5 NavigatorState._flushObserverNotifications (package:flutter/src/widgets/navigator.dart:4333:27)

navigator.dart:4333

6 NavigatorState._flushHistoryUpdates (package:flutter/src/widgets/navigator.dart:4295:5)

navigator.dart:4295

7 NavigatorState._updatePages (package:flutter/src/widgets/navigator.dart:4161:5)

navigator.dart:4161

8 NavigatorState.didUpdateWidget (package:flutter/src/widgets/navigator.dart:3803:7)

navigator.dart:3803

9 StatefulElement.update (package:flutter/src/widgets/framework.dart:5643:55)

..... .. ..

════════ Exception caught by Flutter framework ═════════════════════════════════ 'package:flutter/src/widgets/navigator.dart': Failed assertion: line 2993 pos 18: '!navigator._debugLocked': is not true. ════════════════════════════════════════════════════════════════════════════════ D/newrelic(23857): PayloadController: 0ms. waiting to submit payload [8d20ef85-ac51-4a28-b6aa-c8093ea521dd]. D/newrelic(23857): HarvestTimer: time since last tick: 59997 D/newrelic(23857): Harvest: tick D/newrelic(23857): Harvester state: CONNECTED I/newrelic(23857): Harvester: connected I/newrelic(23857): Harvester: Sending [8] HTTP transactions. I/newrelic(23857): Harvester: Sending [0] activity traces. I/newrelic(23857): Harvester: Sending [0] session attributes. I/newrelic(23857): Harvester: Sending [0] analytics events.

mohadel92 commented 9 months ago

when i remove the observer everything is working fine

ndesai-newrelic commented 8 months ago

@mohadel92 are you still seeing this issue?

ndesai-newrelic commented 7 months ago

@mohadel92 can you check this https://github.com/flutter/flutter/issues/108544?

bernardoveras commented 7 months ago

same problem

duraz0rz commented 7 months ago

Not the OP, but we're seeing a similar issue with using go_router for a modal bottom sheet.

When you use showModalBottomSheet, it can potentially push a route to the Navigator where the settings do not have a route path or name. For us, we're using showModalBottomSheet to pop up a widget, then when the user presses a button on that widget, we navigate to another page.

I believe the difference between Navigator and go_router when NewRelicNavigationObserver intercepts the route is that the Navigator path expects RouteSettings while the go_router path expects a widget.

image

ndesai-newrelic commented 7 months ago

@duraz0rz that's good feedback, i will check it from our side.

duraz0rz commented 7 months ago

To add a bit more to this, we figured out a workaround on our end.

The docs for showBottomModalSheet state that you can pass routeSettings for NavigationObservers:

The optional settings parameter sets the RouteSettings of the modal bottom sheet sheet. This is particularly useful in the case that a user wants to observe PopupRoutes within a NavigatorObserver.

We didn't have this set when we were using Navigator and NewRelicNavigationObserver was not crashing, while it was crashing until we set routeSettings for a bottom sheet modal when we switched to go_router.

ndesai-newrelic commented 7 months ago

@duraz0rz @bernardoveras @mohadel92 can you check this in our latest flutter release?

eugenio-tesio-ueno commented 5 months ago

Is happening again

new_relic: 1.0.8 go_router: 13.2.2 flutter: 3.19.2

Reopen the issue please

workaround:

class RouteSettingsExtends extends RouteSettings {
  const RouteSettingsExtends(
    this.key,
    this.child, {
    super.name,
    super.arguments,
  });
  final String key;
  final Widget child;
}
showModalBottomSheet<Widget>(
    context: context,
    routeSettings: RouteSettingsExtends(
      'Key',
      Container(),
      name: 'bla',
    ),
    child: Container(),
  );
ndesai-newrelic commented 5 months ago

@eugenio-tesio-ueno can you share example app or code snippet for this issue?

MiguelBelotto00 commented 5 months ago

Hello @ndesai-newrelic , I hope you are feeling well.

This actually works for me by changing the child and key parameter that by default it does not bring in the routesettings. before change: code3

after change: code

you know why you are looking for a child or key?.

It seems that the ModalBottomSheets use by default the RouteSettings class that has flutter with only the name, arguments property.

code2

ndesai-newrelic commented 5 months ago

@MiguelBelotto00 Agreed, let's switch to using 'name' instead of 'key'. Could you create a pull request for this change? I'll update the documentation to advise customers to include the 'name' parameter when using the Go router package for routing.

MiguelBelotto00 commented 5 months ago

Yes, for sure I prepare the pr of this.

A good solution would be to stop using the _addGoRouterBreadcrumb and start using only the _addBreadcrumb but I don't know why there are two different functions.

code5

eugenio-tesio-ueno commented 5 months ago

@eugenio-tesio-ueno can you share example app or code snippet for this issue?

@MiguelBelotto00 provided the error found https://github.com/newrelic/newrelic-flutter-agent/issues/68#issuecomment-2059466094

ndesai-newrelic commented 5 months ago

@MiguelBelotto00 that's right, you can remove the method, now we are reading it from the name so we dont need different method.

MiguelBelotto00 commented 5 months ago

Hello me again, another solution that works for me is directly using this.

code2

Reading a little more the code CustomTransitionPage is something from GoRouter, so I guess that's why you have two different functions.

This solution also works because the problem is the is RouteSettings since the base of the RouteSettings does not have a key or child as the MaterialPage or CustomTransitionPage does (The control is already done before using the _addGoRouterBreadcrumb function).i.e:

code4

Do you think this is a valid solution or do I just make the modification of using only the _addBreadcrumb function?

ndesai-newrelic commented 5 months ago

@eugenio-tesio-ueno i am not seeing that issue happening again, bottomsheet is modalBottomSheetRoute and we are not creating breadcrumb for that. if you are still seeing this issue please share example app with us.

ndesai-newrelic commented 5 months ago

Hey @MiguelBelotto00, it seems like route.child is providing the same value as name when passed. If you have any alternative suggestions, feel free to open a PR, and we'll review it.

eugenio-tesio-ueno commented 5 months ago

If you take _addGoRouterBreadcrumb and cast fromRoute and toRoute as RouteSettings, you'll get a syntax error, like this:

image

RouteSetting class.

/// Data that might be useful in constructing a [Route].
@immutable
class RouteSettings {
  /// Creates data used to construct routes.
  const RouteSettings({
    this.name,
    this.arguments,
  });

  /// The name of the route (e.g., "/settings").
  ///
  /// If null, the route is anonymous.
  final String? name;

  /// The arguments passed to this route.
  ///
  /// May be used when building the route, e.g. in [Navigator.onGenerateRoute].
  final Object? arguments;

  @override
  String toString() => '${objectRuntimeType(this, 'RouteSettings')}(${name == null ? 'none' : '"$name"'}, $arguments)';
}
eugenio-tesio-ueno commented 5 months ago

The workaround is to copy the NewRelicNavigationObserver and create your own observer. So I guess this issue isn't critical

ndesai-newrelic commented 5 months ago

@eugenio-tesio-ueno why are you casting this as route settings for from route and to route?

eugenio-tesio-ueno commented 5 months ago

@eugenio-tesio-ueno why are you casting this as route settings for from route and to route?

To convert the dynamic fromRoute variable to RouteSettings type, (previously validated by the if condition) and to show you that key property does not exists.