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

Moved super.dispose down #803

Conversation

ikbendewilliam
Copy link

Fixes #802

@rrousselGit
Copy link
Owner

Hello!

Could you write tests for these changes?

@ikbendewilliam
Copy link
Author

I'm having trouble writing a good test, I can't seem to recreate the same issue as we have in our plugin. I did add a test that validates the delegate isn't disposed before the listeners have been removed though 🤔

@rrousselGit
Copy link
Owner

@ikbendewilliam Did these changes fix the issue with your plugin?

Copy link
Owner

@rrousselGit rrousselGit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but make sure it works on your app first. It'd be a shame if it didn't

@ikbendewilliam
Copy link
Author

@rrousselGit Turns out I'm sick, so when I'm feeling better I'll have another go at writing better tests and I'll confirm that everything is fixed for our plugin. Thank you for your patience

@rrousselGit
Copy link
Owner

No problem! Thanks for your work :)

@ikbendewilliam
Copy link
Author

I have good news and bad news 😅
The good news is that I'm able to reproduce it. The bad news is that I used two providers and the error I was receiving was not because the order of the dispose is wrong, but because dispose got called a second time.
This branch obviously doesn't fix that since the issue is not in the package.

My opinion, however, is still that the order should be first remove listeners and then super.dispose. So for me this can be merged, WDYT?

@rrousselGit
Copy link
Owner

Honestly I'm a bit skeptical.
This could be breaking some folks, since that behavior has been around for ages now.

And the current behavior isn't problematic. You're the first to point it out, I never saw/heard of the issue before.
Removing the listeners after dispose is generally harmless for most provided objects.

@rrousselGit
Copy link
Owner

Hello again!

I don't think I will go through with these changes after-all. In the end:

  • provider is in Long Term Maintenance mode. I avoid to avoid breaking changes
  • this could be silently breaking
  • the value is not that large
  • people haven't requested for this before.

So I'll keep things as they are for now.
But this could be revisited if a large number of people ask for this

I appreciate your effort though!

@rrousselGit rrousselGit closed this Jun 6, 2023
@snaeji
Copy link

snaeji commented Jul 12, 2023

We are in the same situation. It would be very beneficial for us if this would be merged so we don't have to fork 👍

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.

Dispose order is wrong
3 participants