-
-
Notifications
You must be signed in to change notification settings - Fork 909
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
fix!: Allow AssetCache to load json files with array at root #2688
fix!: Allow AssetCache to load json files with array at root #2688
Conversation
@@ -47,9 +47,9 @@ class AssetsCache { | |||
} | |||
|
|||
/// Reads a json file from the assets folder. | |||
Future<Map<String, dynamic>> readJson(String fileName) async { | |||
Future<dynamic> readJson(String fileName) async { |
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.
Hmm... This is a clear degradation with having a completely dynamic type here (and a breaking change), maybe we should just have another method to load lists.
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 would strongly argue against that. For a few reasons:
- JSON is allowed to be both an array or object at the root
- The native decodeJson implementation returns
dynamic
for this reason - A user may not know what the JSON looks like before they load it, and will then not know which method to choose ahead of time (this is what I personally ran into)
- Implementations that use JSON already expect to cast the internal JSON structures to the correct format - see below
This example is from the "aseprite" animation example that I 'fixed' as part of this PR. In the PR, I cast the dynamic JSON to a Map<String, dynamic>
before passing to fromAsepriteData
. I believe this is the correct approach. As soon as you go into the method you can see that the internal dynamic
structures are cast to Map
as appropriate (and would be cast to List
as well if there were any). So this is already something that consumers of JSON expect to have to do.
factory SpriteAnimation.fromAsepriteData(
Image image,
Map<String, dynamic> jsonData,
) {
final jsonFrames = jsonData['frames'] as Map<String, dynamic>; // Projectitis: here
return SpriteAnimation(
jsonFrames.values.map((dynamic value) {
final map = value as Map;
final frameData = map['frame'] as Map<String, dynamic>; // Projectitis: here
final x = frameData['x'] as int;
final y = frameData['y'] as int;
final width = frameData['w'] as int;
final height = frameData['h'] as int;
final stepTime = (map['duration'] as int) / 1000;
final sprite = Sprite(
image,
srcPosition: Vector2Extension.fromInts(x, y),
srcSize: Vector2Extension.fromInts(width, height),
);
return SpriteAnimationFrame(sprite, stepTime);
}).toList(),
);
}
If a user requires a strongly typed JSON implementation, a better approach (for them) would be to look at JSON code generation.
If you did want to go ahead with separate methods for object and array based JSON files, I would suggest:
- Keep
dynamic readJson
as it is - Add
Map<String, dynamic> readJsonObject
, and - Add
List<dynamic> readJsonArray
Thoughts?
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.
Even though it obviously is the preferred way to accept all valid json structures, we have to take into consideration that many have already done their implementations on top of the current function definition so we should really try to avoid creating a breaking change for this.
If you did want to go ahead with separate methods for object and array based JSON files, I would suggest:
This suggestion keeps the breaking change. I think only step 3 should be done here, and then the breaking change can be done for v2.
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.
How about introducing dynamic loadJson
and marking readJson
as deprecated?
I notice all the AssetCache
methods are read
and all the Images
methods are load
.
This could be a step towards standardising on load
?
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.
Yeah, that could be a possibility.
What do you think @flame-engine/flame-admin?
I would introduce both 2 and 3 too, so that the user can do one less cast.
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 propose that we leave this one until v2 or there could be a bunch of confusion. JSON with an array at the root does not seem very common (or it would have come up before now). I am using jsonDecode
directly to get around this issue myself.
But how do we mark something so it's not forgotten in v2?
A workaround for those who run into this issue and still want to use AssetsCache to take advantage of caching is:
AssetsCache _assets = AssetsCache():
@override
Future<void> onLoad() async {
/// Instead of
final json = await _assets.readJson('data.json');
// Do one of these
final json = jsonDecode(await _assets.readFile('data.json'));
final json = jsonDecode(await _assets.readFile('data.json')) as Map<String, dynamic>;
final json = jsonDecode(await _assets.readFile('data.json')) as List<dynamic>;
}
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.
But how do we mark something so it's not forgotten in v2?
We add it to the #1938 ticket, I added it now :)
Should we close this for now then?
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.
Yup, go ahead :)
Closing since we came to the conclusion that this should wait until v2. |
Description
AssetCache.readJson
to allow reading json files with either an object or an array at the root.Checklist
docs
and added dartdoc comments with///
.examples
ordocs
.Breaking Change?
In some cases the user will now need to cast the JSON object to a
Map<String, dynamic>
where this datatype is expected in an upstream function. A good example of this isexamples\lib\stories\animations\aseprite_example.dart
where the Json is loaded, then passed intoSpriteAnimation.fromAsepriteData
which expects aMap<String, dynamic>
and not adynamic
.Related Issues
Closes #2682