Skip to content
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

Add scopeName parameter to popScope #292

Merged
merged 8 commits into from
May 7, 2023

Conversation

olexale
Copy link
Contributor

@olexale olexale commented Aug 25, 2022

Let's imagine an object (it could be a complex stateful widget, a page, or an anchor in AR) that has complex internal dependencies (object-scoped services, repositories, etc.). I found it highly convenient to push a separate named scope during this object creation and lazily initialize its dependencies. The problem is that, when the object dies, I cannot remove the scope without popping scopes that potentially were pushed on top of it, i.e. it is not possible to drop the scope B without dropping scope C as well:

[scope A] -> [scope B] -> [scope C]

With this PR I propose adding functionality that allows dropping any named scope from the stack.

@escamoteur
Copy link
Collaborator

I know that it's been a long time, but I had to make a longer break due to health issues.
Wouldn't that be more a disposeScope than a popScope?

@olexale
Copy link
Contributor Author

olexale commented May 4, 2023

In the beginning, I named the method dropScope, I cannot recall the exact reason for relocating the implementation to popScope and introducing a parameter. disposeScope also seems logical to me.

I'm happy to refactor the proposed implementation. Which method name would you prefer? Should we go with disposeScope?

@escamoteur
Copy link
Collaborator

Yeah, I think disposeScope makes sense because it is probably called because people want to dispose of all objects in it.
may I also invite you to the discussion here: escamoteur/watch_it#1

@escamoteur escamoteur mentioned this pull request May 5, 2023
@escamoteur
Copy link
Collaborator

thinking a bit more (toilet break :-) ) dropScope might indeed be better because it transports better the meaning that the scope is not only dropped but also disposed.

@escamoteur
Copy link
Collaborator

It would be awesome if you also could add the new function to the readme

@olexale
Copy link
Contributor Author

olexale commented May 5, 2023

I extracted the functionality into dropScope and updated the readme. Unfortunately, tests are failing now, but I see they are failing in the master branch, so I assume it's a common problem.
I'll investigate why this happens later today if the issue is not resolved before that.

@olexale
Copy link
Contributor Author

olexale commented May 5, 2023

It looks like tests are failing because asserts were replaced with throwIfNot (like in this commit). I see that these changes were added 5 hours ago, so you are probably working on them now. To make tests pass locally I just replaced

throwsA(const TypeMatcher<AssertionError>()),

with

throwsA(const TypeMatcher<StateError>()),

I'm happy to raise additional PR with the fix if needed.

@escamoteur escamoteur merged commit 73bc89f into fluttercommunity:master May 7, 2023
2 checks passed
@escamoteur
Copy link
Collaborator

Thanks a lot for this PR

@escamoteur
Copy link
Collaborator

Included in just pushed V7.5.0

@olexale
Copy link
Contributor Author

olexale commented May 8, 2023

Thank you!

@escamoteur
Copy link
Collaborator

thanks to your PR I now could finally make pushScope in the get_it_mixin more secure https://github.com/escamoteur/get_it_mixin/blob/master/CHANGELOG.md#420---22032023

@olexale
Copy link
Contributor Author

olexale commented May 27, 2023

We use functionality similar to pushScope from the get_it_mixin a lot. Without it, the whole codebase was full of dependency overrides, and I needed to track and align our get_it fork with the original.
Thanks for merging the PR, this simplified my life a bit 🙂

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

Successfully merging this pull request may close these issues.

None yet

10 participants