Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Reorganize chart pane controller code and prepare structure for offline mode. #7572
Reorganize chart pane controller code and prepare structure for offline mode. #7572
Changes from 106 commits
e522c7e
ebded8c
7976e00
3c87709
ea47bc9
3bbed40
215ed02
f5a0acc
dc0844f
94ad638
b6e8225
b73eb28
5067dbc
c31d051
51a189a
63daf74
aeeba2d
08de780
372d822
3ca3cd1
bcc0246
f8f29ce
39b5695
bcd2514
0724dba
741f0c4
3e28d5c
f01ced6
7a29b65
4ebae9e
0771789
cf4b341
ba0e76d
2ceab20
b230792
f64f826
f1e7e5e
9601724
8193e9e
38bc77a
dbf826a
c58ce81
9ad3f28
ad3cb08
0da6798
9068996
d5633af
0a91214
cbd931f
ee7503f
e9098b1
64f1961
c4dac54
10b62cf
bd0df00
b2d8f22
ce96556
b8c4f35
d2a1254
8f0010b
6ee54a2
9b2c45a
d58cc26
2aaa8dc
ef8ce6a
c91f7c2
a8eac4e
6b9510e
a5cfeef
575497b
8d4a249
fab3a2b
ce0ee73
3aea57c
815c66e
694a1df
7999d3c
a56cc2e
f7e3c64
407fb59
74d0656
e869a70
c4993c5
e97047d
3b87715
a897a1f
8b35071
f592d2f
faab035
48a43b7
789c2f7
1caa0fe
8ea8e39
444fa53
fb1ffa8
e0093a4
0ec2767
b96eb76
324d0d4
245fad1
709f425
b4a4166
9b8518b
20ebec2
5fc768d
5d7d3a5
8938b89
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use
Object?
instead ofdynamic
nit: how about naming
toJson
for consistency with the rest of the codebase? This can also be a getter `Map<String, Object?> get toJson => {...};There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is consistent with OfflineMemoryData.
I would update both with what is recommended for serialization: https://docs.flutter.dev/data-and-backend/serialization/json#serializing-json-inside-model-classes
And we can make them extensions.
How does it sound?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If is recommended to never use
dynamic
. We should be usingObject?
instead. These have different implications in the compiler and for type checking. Couldn't find official docs but here's an issue with some discussion on this topic: dart-lang/language#3192There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest to do it as separate clean up, including
OfflineMemoryData.parse(Map<String, dynamic> json)
and others.I expect unexpected side effects that may cause us to change our mind here. I remember trying to use Object for json instead of dynamic and facing some difficulties that convinced me to stay with standard for json.
Serialization is a specific mechanism that picks up all objects that meet this standard: https://docs.flutter.dev/data-and-backend/serialization/json#serializing-json-inside-model-classes
This mechanism may break with switch to Object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@srawlins do you know if the linked docs above are up to date? I know you've done a pretty large overhaul of all the JSON parsing we were doing in DevTools using extension types. Can you advise here WRT using
dynamic
vsObject?
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the docs are still accurate. Anything produced by
dart:convert
usesdynamic
(e.g.List<dynamic>
andMap<String, dynamic>
or maybe evenMap<dynamic, dynamic>
) instead ofObject?
.We can also reference the general recommendation to avoid
dynamic
, and I stick strongly to that. I avoiddynamic
anywhere it isn't necessary. So then then question is: when working with objects produced bydart:convert
, what should we use? I thinkObject?
is safe everywhere here, but I actually haven't worked through the edge cases, etc. I can't promise it, so I think I useddynamic
in a lot of my devtools PRs, just for consistency.The runtime danger is that the objects returned from
dart:convert
don't "promote" types of objects when you might expect it. For example, if you callx = jsonDecode('["hello", "there"]');
, you get aList<dynamic>
thing at runtime, and you might think "well, I know it's always a List of Strings, so I'll usex as List<String>
." That is ok at compile-time, but will throw at runtime. You might tryif (x is List<String>)
as defensive, but now that condition will never be true at runtime. It's super awkward but you have to doif (x is List) x.cast<String>()
(which could still throw if you have bad data) or (if (x is List) for (var y in x) if (y is String) ...
). Super tedious.Back to
Object?
vsdynamic
. Either of these will work, I think all the time, but I'm not sure. E.g.This prints "yay a List" and "yay a List<Object?>" because of how is-checks work (assignability?). But not "yay a List". That test fails. And the calls to
f1
andf2
pass, but the call tof3
throws an exception, because the returned object is not aList<Object>
.I think guidance w.r.t. dynamic-and-JSON is a team-by-team decision. We can't all become type-system experts and JSON experts (like I can never if JSON explicitly does not support a
null
concept, or does, or if Dart does something special...). So whatever consistent policy keeps the team writing safe code and understandable (for the team) code, is a good policy. I don't think analyzer team has a consistent policy, and we definitely could have JSON-parsing bugs, or YAML-parsing bugs, in malformed analysis_options.yaml, or package_config.json, etc. It's something I keep punting on, hopingdart:convert
will just change to returningObject?
objects instead ofdynamic
, or maybe a better JSON-parsing story. 🤷There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I suggest to stay compliant with public recommendation.
If we want to invest into switch to <String, Object?>, we need to reflect it in our style rules and migrate the entire project at once, to keep it consistent.