Should didUpdateProvider receive an AsyncError while providerDidFail can handle it now?

[AhmedLSayed9]

ProviderObserver.providerDidFail now handles Exceptions from asynchronous providers (#3067) but didUpdateProvider still receive AsyncErrors too, which might lead to issues/duplicates. Is there a reason for that?

I'd like to fix this if it's not intended.

[rrousselGit]

That's consistent with how the observer works.
If the provider throws synchronously on creation, both providerDidFail and didAddProvider would be invoked.

Changing it would've been breaking anyway. So I don't think it's worth changing

[AhmedLSayed9]

If the provider throws synchronously on creation, both providerDidFail and didAddProvider would be invoked.

I think if didUpdateProvider won't receive AsyncErrors, this should be updated too so only providerDidFail would be invoked.

The common use case for ProviderObserver is to log states and report errors. With the current behavior we have to do:

class ProviderLogger extends ProviderObserver {
  @override
  void didAddProvider(
    ProviderBase<Object?> provider,
    Object? value,
    ProviderContainer container,
  ) {
    // We'll have to add this to avoid duplicate logs or other logic execution.
    if (value is Exception) return;

    log('TODO: log provider and the state');
  }

  @override
  void didUpdateProvider(
    ProviderBase<dynamic> provider,
    Object? previousValue,
    Object? newValue,
    ProviderContainer container,
  ) {
    // We'll have to add this to avoid duplicate logs or other logic execution.
    if (newValue is AsyncError) return;

    log('TODO: log provider and the state');
  }

  @override
  void providerDidFail(
    ProviderBase<dynamic> provider,
    Object error,
    StackTrace stackTrace,
    ProviderContainer container,
  ) {
    FirebaseCrashlytics.instance.recordError(error, stackTrace, fatal: true, reason: provider.name);

    log('TODO: log provider and the error/stackTrace');
  }
}

imo, the default behavior should handle that and have a single source of truth.

Furthermore, the current behavior of providerDidFail receive the error object instead of AsyncError (if thrown from an async provider), which is fine, but the previousValue could be helpful to track some issues.

As discussed at #3145 receiving an AsyncError might not be a good idea as we'll have duplicated StackTrace, but providerDidFail can alternatively receive a nullable previousValue, which will also be helpful if the error is thrown from a sync provider (after the first build).

The final result would be:

class ProviderLogger extends ProviderObserver {

  // This will not trigger if an exception is thrown on creation.
  @override
  void didAddProvider(
    ProviderBase<Object?> provider,
    Object? value,
    ProviderContainer container,
  ) {
    log('TODO: log provider and the state');
  }

  // This will not trigger if the newValue is an exception/AsyncError
  @override
  void didUpdateProvider(
    ProviderBase<dynamic> provider,
    Object? previousValue,
    Object? newValue,
    ProviderContainer container,
  ) {
    log('TODO: log provider and the state');
  }

  @override
  void providerDidFail(
    ProviderBase<dynamic> provider,
    Object? previousValue,
    Object error,
    StackTrace stackTrace,
    ProviderContainer container,
  ) {
    FirebaseCrashlytics.instance.recordError(
      error,
      stackTrace,
      fatal: true,
      reason: provider.name,
      information: [previousValue], //Reporting previous value can be helpful
    );

    log('TODO: log provider and the error/stackTrace');
  }
}

[lucavenir]

imo I'd love a breaking change better than having ambiguities or surprises.

Maybe plan this for v3?

[rrousselGit]

There's no ambiguity here. The behavior is intended. There's more than just logging as a use case for observers.
In particular, ProviderObserver was built in mind with the ability to allow writing a devtool.

Maybe it should be updated to make logging specifically simpler. But IMO that's not a huge concern.
It's not like people write 50 loggers in their applications. There's a limit to the value here