Skip to content

Potential Issue with Scoped Instances when Multiple Pages are Displayed Simultaneously #376

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

Closed
zhujiageng opened this issue Sep 25, 2024 · 21 comments

Comments

@zhujiageng
Copy link

Hello,

I'm encountering a potential issue with GetIt's scope management that allows multiple instances of the same page type to be displayed simultaneously. I'd like to describe my scenario and ask for confirmation on whether this is a limitation of GetIt or if there's a recommended workaround.

Scenario:

I have a UserProfilePage that displays a user's profile information. This page can display any user's profile, including the current user's or any other user's profile.
From any user's profile page, I can navigate to another user's profile page, and so on. This means I can have a navigation stack with multiple UserProfilePage instances, potentially showing the same user's profile multiple times in different instances.
Each UserProfilePage needs its own independent UserProfileController to manage its state, as each page instance should maintain its own state and not interfere with others, even if they are displaying the same user's profile.

Concern:

When a new UserProfilePage is pushed onto the navigation stack, a new scope is created, and a UserProfileController is registered within that scope.
However, when the user navigates back (e.g., via a back gesture or side-swiping), there can be a moment where two UserProfilePage instances are simultaneously active (the one being popped and the one underneath).
If the lower UserProfilePage's widget tree rebuilds during this time (e.g., due to setState or other reasons), its child widgets will attempt to retrieve the UserProfileController.
Since GetIt's scopes are global and stack-based, and the most recently pushed scope is on top, the child widgets in the lower page will retrieve the UserProfileController from the top scope, which belongs to the upper page.
This means that the lower page's widgets might end up using the wrong controller instance, leading to incorrect behavior.

Question:

Is this understanding correct? Does GetIt's scope mechanism cause child widgets in lower pages to retrieve instances from the wrong scope when multiple scopes are active due to navigation behaviors like side-swiping back?
If so, is there a recommended way to handle this scenario with GetIt, ensuring that each page's widgets retrieve their controller instances from the correct scope?
Alternatively, is this a known limitation of GetIt's scope mechanism when used in Flutter applications with navigation stacks and overlapping pages?

Additional Context:

I understand that GetIt scopes are global and not tied to Flutter's BuildContext, which might be causing this issue.
I'd prefer to continue using GetIt if possible, so any guidance or suggestions on how to properly manage per-page scoped instances in this scenario would be greatly appreciated.

Thank you for your assistance!

@escamoteur
Copy link
Collaborator

escamoteur commented Sep 25, 2024

Hi, we actually had a exact the same requirement in my current project. The solution is not to use scopes but instead use named registrations of you UserProfileController. Register it with your userId or userName as instanceName. I recently added especially to make this sort easier to deal with the following new apis:

  /// Only registers a type new as Singleton if it is not already registered. Otherwise it returns
  /// the existing instance and increments an internal reference counter to ensure that matching
  /// [unregister] or [releaseInstance] calls will decrement the reference counter an won't unregister
  /// and dispose the registration as long as the reference counter is > 0.
  /// [T] type/interface that is used for the registration and the access via [get]
  /// [factoryFunc] that is callled to create the instance if it is not already registered
  /// [instanceName] optional key to register more than one instance of one type
  /// [dispose] disposing function that is autmatically called before the object is removed from get_it
  T registerSingletonIfAbsent<T extends Object>(
    T Function() factoryFunc, {
    String? instanceName,
    DisposingFunc<T>? dispose,
  });

and

  /// checks if a regiserter Singleton has an reference counter > 0
  /// if so it decrements the reference counter and if it reaches 0 it
  /// unregisters the Singleton
  /// if called on an object that's reference counter was never incremented
  /// it will immediately unregister and dispose the object
  void releaseInstance(Object instance);

if needed there is also

  /// In some cases it can be necessary to change the name of a registered instance
  /// This avoids to unregister and reregister the instance which might cause trouble
  /// with disposing functions.
  /// IMPORTANT: This will only change the the first instance that is found while
  /// searching the scopes.
  /// If the new name is already in use in the current scope it will throw a
  /// StateError
  /// [instanceName] the current name of the instance
  /// [newInstanceName] the new name of the instance
  /// [instance] the instance itself that can be used instead of
  /// providing the type and the name. If [instance] is null the type and the name
  /// have to be provided
  void changeTypeInstanceName<T extends Object>({
    String? instanceName,
    required String newInstanceName,
    T? instance,
  });

So when you display a userprofile you call registerSingletonIfAbsent and in its dispose you call releaseInstance on the result that you got from registerSingltonIfAbsent

let me know if that solves your problem

@zhujiageng
Copy link
Author

In fact, the same user's profile may appear multiple times, such as UserA > UserB > UserC > UserD > UserB. Using user ID or user name would result in different pages obtaining the same page state for the same user, whereas they should actually be different.

@escamoteur
Copy link
Collaborator

so your controller maintains state that is different between two displays of the same user profile?

@escamoteur
Copy link
Collaborator

Why would you want to store such state in get_it and not in the state of the widget?

@zhujiageng
Copy link
Author

Yes, each page needs an independent controller, even if their user IDs are the same. This is because the controller holds objects like ScrollController, which cannot be simply reused between multiple pages.

@escamoteur
Copy link
Collaborator

Why not store this controllers in the Widget State? IMHO this is clearly UI only and should not be stored outside the widgettree if it is not shared between two widget instances

@zhujiageng
Copy link
Author

For example, when a button on the page is clicked to scroll the page to a specific location, the button's event is bound to the controller's method. Therefore, the controller needs to hold a ScrollController to accomplish the scrolling.

@escamoteur
Copy link
Collaborator

escamoteur commented Sep 25, 2024

but why put that into a controllerobject inside getit and not store that in the state of your profile page widget?

@zhujiageng
Copy link
Author

Because the button click is not just about scrolling the page; it also involves sending Mixpanel events, recording logs, and so on. If these actions are written directly into the widget, it would mean that the separation of UI and logic has not been achieved.

@zhujiageng
Copy link
Author

Additionally, something like a RefreshController also needs to be held within the controller, so that when the network request within the controller finishes, the refresh status can be closed.

@escamoteur
Copy link
Collaborator

No, I think you are drawing the separation at the wrong point. Everything that holds state that is only used in one widget is best stored in the widgets state. Additionally you should have some business object that handled things like the rerefresh that is not linked to one specific widget in get_it and can get called from functions located in the widgets state.

@escamoteur
Copy link
Collaborator

The idea of Scopes is to be able to separate business logic states , not widget's states, for that we have the State of Stateful Widgets

@zhujiageng
Copy link
Author

If the ScrollController is managed by the widget, how can the above logic be implemented? For example, some logic in a button click needs to wait for the scrolling to finish. If the controller holds it directly, a single await animateTo would suffice. However, if it doesn't have access to the ScrollController, this cannot be achieved, or it would have to be implemented through a much more complex process of repeatedly sending events.

@zhujiageng
Copy link
Author

You think that widget state is not business logic, but I believe this perception is incorrect. For front-end frameworks like Flutter, UI logic is a very important type of business logic. And much of the UI logic is implemented through objects such as ScrollController and RefreshController.

@escamoteur
Copy link
Collaborator

what should happen after the finish of the scroll?

@zhujiageng
Copy link
Author

When a user first sees a particular view, we will pop up a tip tutorial. This view is not originally within the viewport but is seen after scrolling, so we need to wait for the scrolling to finish.

@escamoteur
Copy link
Collaborator

ok, so why not have the await animateTo in the widget's state and call from there some TutorialManager that is registered inside get_it? The scrolling part is pure UI and only related to that one widget.

@zhujiageng
Copy link
Author

sending mixpanel event, printing a log, scrolling to a specific location, and then popping up a tip tutorial constitute the complete event handling logic for this button. When the controller holds the ScrollController, this logic is within a single method, which is very cohesive. If the widget holds the ScrollController instead, the click event in the controller would need to emit an event, which the widget would then listen to, and then the widget would call TutorialManager within itself. This would make the logic scattered and hard to maintain, because actual business logic is often more complex, and this approach would result in spaghetti code.

@escamoteur
Copy link
Collaborator

that completely depends on your overall app design. Please stop lecturing me about what produces spaghetti code. I'm doing architecture for almost 30 years now.
If you push a new scope for every new profile page to store a new instance of your controller there is no conceptual difference to create that controller in initState and storing it in the widget's state.
you should store objects in get_it that are used from multiple widgets. Depending on which state management you are using there are a lot of different options to implement this.
The problem you describe is not because of a limitation of get_it, but in your current structure of your code.

@zhujiageng
Copy link
Author

I sincerely apologize if I have inadvertently offended you. I greatly admire your creativity. I will close the issue.

@escamoteur
Copy link
Collaborator

If you engage into discussion and provide factual arguments, I'm more than willing to help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants