luis901101 / oauth_webauth

BSD 3-Clause "New" or "Revised" License
15 stars 16 forks source link

[Dart 3] Replace window singleton instance with view based on build context #12

Closed frederikstonge closed 1 year ago

luis901101 commented 1 year ago

Hi @frederikstonge thanks for this PR. The changes you propose are breaking as BuildContext is now required on all those functions, I'm not sure right now if this would be the best way, I have to check. Anyway if this were the way to go, version should be raised as a major version update not a fix update.

frederikstonge commented 1 year ago

As per the breaking change documentation of flutter 3.10.0 : https://docs.flutter.dev/release/breaking-changes/window-singleton

We need to either use View.of(context) (recommended), or WidgetsBinding.instance.platformDispatcher, which exposes a nullable implicitView, or a list of views.

implicitView's documentation is the following:

The [FlutterView] provided by the engine if the platform is unable to create windows, or, for backwards compatibility.

If the platform provides an implicit view, it can be used to bootstrap the framework. This is common for platforms designed for single-view applications like mobile devices with a single display.

Applications and libraries must not rely on this property being set as it may be null depending on the engine's configuration. Instead, consider using [View.of] to lookup the [FlutterView] the current [BuildContext] is drawing into.

While the properties on the referenced [FlutterView] may change, the reference itself is guaranteed to never change over the lifetime of the application: if this property is null at startup, it will remain so throughout the entire lifetime of the application. If it points to a specific [FlutterView], it will continue to point to the same view until the application is shut down (although the engine may replace or remove the underlying backing surface of the view at its discretion).

So I guess we have two options :

  1. We use the static WidgetsBinding.instance.platformDispatcher.implicitView, cross our fingers it is defined, and keep the version as is.
  2. We pass the BuildContext to use View.of(context) and increase the major version since it is a breaking change.

Maybe a better solution can be done, since it's only used in the clear cache method. I didn't look deeper.

luis901101 commented 1 year ago

I'm not saying to completely remove BuildContext from params but to set it as optional, so you can pass the context and be used as recommended, but maybe for some use case in some app there is no BuildContext available to pass it, so it's up to the developer to decide if it want to use the clearCache without a context.

frederikstonge commented 1 year ago

I've made BuildContext optional like you said, with a fallback on the implicitView, and another fallback if null on the first item in views.

luis901101 commented 1 year ago

Great, I will be merging and releasing new version soon