Convert Diagnosticable to a mixin#51495
Conversation
fluttergithubbot
added
a: accessibility
gspencergoog
force-pushed
the
remove_diagnosticable
branch
from
baa502e to
16c4f82
Compare
gspencergoog
requested review from
darrenaustin
and removed request for
HansMuller
darrenaustin
left a comment
darrenaustin
left a comment
There was a problem hiding this comment.
Nice. Too bad we can't compose mixins, as that would have removed some duplication between Diagnosticable and DiagnosticableMixin.
Otherwise, it LGTM.
| const Diagnosticable(); | ||
| /// _This mixin is exists only to support plugins that require older Flutter | ||
| /// versions: Use the identical mixin [Diagnosticable] instead for most code._ | ||
| mixin DiagnosticableMixin implements Diagnosticable { |
There was a problem hiding this comment.
Given that we intend to remove this, should it be marked as deprecated?
There was a problem hiding this comment.
Yes, I definitely considered that, but mainly, I didn't want a plugin that wants to work with both master and stable to start throwing deprecation warnings on master: some projects would consider that an error and so they could no longer use that plugin on master, and the plugin author couldn't do anything about it until this change hit stable. So I decided to wait to deprecate it until the change reached stable.
There was a problem hiding this comment.
There's a trailing _ here. Also: "most code" could probably use some clarification.
There was a problem hiding this comment.
That's no trailing _, that's the end of the italics for the paragraph.
|
I'd starting reviewing this some time ago and then neglected to even submit what I'd written. So FWIW, now that it's too late :-) |
|
Did your writings uncover anything I should change? |
|
(Oh, I had to refresh GitHub to see them) |
|
The point about "most code" might be worth considering. I suppose youu mean all new code? https://github.com/flutter/flutter/pull/51495/files/baa502ed72e8ebdb12ba13d5687ee37d7d03f9c0#diff-e656bb511c2aa740e42637a30e61aabc |
|
The only code that needs to use that is plugin/package code that needs to work on stable. I'll clarify that, though. |
Just some very minor doc changes: an adjustment of my comments in #51495, and a typo fix.
|
@gspencergoog this is breaking builds for apps that use this plugin: https://github.com/Realank/flutter_datetime_picker Is there any migration guide to update the plugin? |
|
@Zazo032 Sorry about that. There's no migration guide yet, but I'll make one today. It's pretty straightforward, though: If the plugin should be compatible with the class MyClass extends Diagnosticable { ...to this: class MyClass with DiagnosticableMixin { ...At a future time (once this change lands in class MyClass with Diagnosticable { ...and avoid the churn. On the up side, this change also decreased code size by 1.3%... |
|
No problem, I filled a PR to fix the plugin. Thanks for the size reduction! It was 0.2mb on our appbundle 🔥 |
|
This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of |
6 participants
Description
This converts
Diagnosticableto be a mixin instead of an abstract class, so that it can be used to add diagnostics to classes which already have a base class.It leaves in place the
DiagnosticableMixinmixin, since there are some plugins that are still using it, and removing it would mean that those plugins wouldn't work with master branch Flutter anymore.DiagnosticableMixinwill be removed once this mixin version ofDiagnosticablemakes its way to the stable branch.Related Issues
Tests
Breaking Change
Diagnosticable.