fix!: Allow AssetCache to load json files with array at root

[projectitis]

Description

• Fix AssetCache.readJson to allow reading json files with either an object or an array at the root.
• Added simple tests for both scenarios.

Checklist

• I have followed the Contributor Guide when preparing my PR.
• I have updated/added tests for ALL new/updated/fixed functionality.
• I have updated/added relevant documentation in docs and added dartdoc comments with ///.
• I have updated/added relevant examples in examples or docs.

Breaking Change?

• Yes, this PR is a breaking change.
• No, this PR is not a 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 is examples\lib\stories\animations\aseprite_example.dart where the Json is loaded, then passed into SpriteAnimation.fromAsepriteData which expects a Map<String, dynamic> and not a dynamic.

Related Issues

Closes #2682

[spydon]

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.

[projectitis]

I would strongly argue against that. For a few reasons:

1. JSON is allowed to be both an array or object at the root
2. The native decodeJson implementation returns dynamic for this reason
3. 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)
4. 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:

1. Keep dynamic readJson as it is
2. Add Map<String, dynamic> readJsonObject, and
3. Add List<dynamic> readJsonArray

Thoughts?

[spydon]

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.

[projectitis]

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?

[spydon]

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.

[projectitis]

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>;
}

[spydon]

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?

[projectitis]

Yup, go ahead :)

[spydon]

Closing since we came to the conclusion that this should wait until v2.