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

Remove unused Chain.disable functionality #121

Open
natebosch opened this issue Oct 18, 2022 · 3 comments
Open

Remove unused Chain.disable functionality #121

natebosch opened this issue Oct 18, 2022 · 3 comments
Labels
next-breaking-release Issues that are worth doing but need to wait for a breaking version bump

Comments

@natebosch
Copy link
Member

I cannot find any uses of the Chain.disable API outside of the tests in this package. It is unnecessary complexity. Removing the functionality may also remove some of the overhead from this package since we can omit a boolean check and reading a zone variable for every zone callback.

Any concerns @jakemac53 @lrhn ?

@natebosch natebosch added the next-breaking-release Issues that are worth doing but need to wait for a breaking version bump label Oct 18, 2022
@natebosch
Copy link
Member Author

Hmm, we might not be able to do a major version release because of flutter pinning. 😢

@lrhn
Copy link
Member

lrhn commented Oct 19, 2022

No real opinion on removing the behavior. Sure, go for it!

The implementation looks potentially unsafe. If you disable stack chaining using this function, then start a new stack chaining inside it, then the value won't be correct. (It will enable the outer stack chaining zone as well).

The test, if anything, should be if (!identical(this, zone[Chain._specKey])) return parent.registerCallback(zone, f);,
and then you don't need the extra boolean zone-variable.

(Don't read Zone.current in a zone callback, always use the zone parameter. A subzone might have chosen to change that argument deliberately.)

@jakemac53
Copy link
Contributor

If there is no known usage, I would be ok with removing this as a minor version bump. It will likely cause less pain than revving the whole ecosystem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
next-breaking-release Issues that are worth doing but need to wait for a breaking version bump
Projects
None yet
Development

No branches or pull requests

3 participants