make some BuildContext methods generics #44189

a14n · 2019-11-05T16:40:56Z

Description

Several methods in BuildContext were using Type to search for ancestor.

inheritFromWidgetOfExactType
ancestorInheritedElementForWidgetOfExactType
ancestorWidgetOfExactType
ancestorStateOfType
rootAncestorStateOfType
ancestorRenderObjectOfType

Most of those methods implied a cast at call site because their return type was a parent type. Moreover the type provided was not checked at analysis time even if the type is actually constrainted.

Making those methods generics improves type safety and need less code.

// before
ComplexLayoutState state = context.ancestorStateOfType(const TypeMatcher<ComplexLayoutState>()) as ComplexLayoutState;
// after
ComplexLayoutState state = context.ancestorStateOfType<ComplexLayoutState>();

Related Issues

None

Tests

None because it's a refactoring.

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I signed the CLA.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I updated/added relevant documentation (doc comments with ///).
All existing and new tests are passing.
The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require Flutter developers to manually update their apps to accommodate your change?

Yes, this is a breaking change (Please read Handling breaking changes). Replace this with a link to the e-mail where you asked for input on this proposed change.
No, this is not a breaking change.

goderbauer · 2019-11-06T18:18:00Z

I really like this clean-up and the extra type safety it gives us. However, this looks like a rather big breaking change that will also break many users of flutter (see current cirrus failures as an example). I wonder if we can somehow do this change with a softer transition where we deprecate the existing API, but it continuous to work until our dependencies had a chance to upgrade to the new API.

a14n · 2019-11-06T20:24:02Z

I really like this clean-up and the extra type safety it gives us.

Me too :)

this looks like a rather big breaking change that will also break many users of flutter

Yes :( but the upcoming NNBD will be way more breaking.

see current cirrus failures as an example

I planned to update scoped_model but I was wondering how to deal with that (flutter repo depends on scoped_model and scoped_model depends on flutter - perhaps make a git ref dependency on a fork of scoped_model until a new flutter version is out)

I wonder if we can somehow do this change with a softer transition where we deprecate the existing API, but it continuous to work until our dependencies had a chance to upgrade to the new API.

I thought about it and I didn't see a good solution:

make Type parameter optional could break overrides and is not possible with inheritFromWidgetOfExactType that already have an optional named parameter.
duplicate methods needs to have new method names (the current method names are good IMO) and deprecates old methods

I don't really know how often those methods are used outside of flutter but if someone has access to a corpus of code on pub it could be really interesting to grab some datas on the usage of those methods.

WDYT?

a14n · 2019-11-07T15:36:05Z

It's now green with a patched version of scoped_model.

Hixie · 2019-11-12T19:17:02Z

Can we make this less breaking by deprecating these versions and instead adding new methods with different (more uniform?) names that use the new style?

e.g. we could add "find" to those that are O(N), to make it clearer that they're more expensive.

I'm not sure exactly what to do with the O(1) ones...

Old	New
inheritFromWidgetOfExactType	?
ancestorInheritedElementForWidgetOfExactType	?
ancestorWidgetOfExactType	findAncestorWidgetOfExactType
ancestorStateOfType	findAncestorStateOfType
rootAncestorStateOfType	findRootAncestorStateOfType
ancestorRenderObjectOfType	findAncestorRenderObjectOfType

goderbauer · 2019-11-12T19:36:41Z

I like the suggestion regarding the "find".

Here are my suggestions for the other two. I am hoping that "depend" makes it more clearer than "inherit" that you're adding a dependency on the InheritedWidget and get rebuilt whenever it changes. The other method's name just adopted to that naming scheme and the "get" sets it apart from the "find" in terms of lookup performance.

inheritFromWidgetOfExactType -> dependOnInheritedWidgetOfExactType
ancestorInheritedElementForWidgetOfExactType -> getElementForInheritedWidgetOfExactType

a14n · 2019-11-12T20:30:00Z

find* looks good to me.

Let me know what the other 2 methods should become.

Hixie · 2019-11-12T23:42:08Z

@goderbauer's proposals LGTM.

goderbauer · 2019-11-12T23:43:36Z