[4.0] Change LottieConfiguration to default to DecodingStrategy. dictionaryBased

[marcelofabri]

Since 4.0 is going to be a breaking change, we probably should change LottieConfiguration to default to DecodingStrategy. dictionaryBased as it should be always faster.

I can put up a diff later if this is something we agree on.

[calda]

I'm not entirely sure that I'm ready to make this change quite yet. Some thoughts:

1. I think we don't have any snapshot tests for the .dictionaryBased implementation. Last time I looked at enabling .dictionaryBased by default, I think I got a fair number of test failures. We'd need to update the snapshot tests to cover this first.
2. I'm not sure how extensively the new code has been tested in production. I haven't gotten any bug reports about this implementation yet with either means that there aren't any bugs or that there haven't been many people testing this yet. If we don't find any issues when enabling snapshot testing then this may not be an issue. I'd personally like to try enabling this by default in Airbnb's iOS app (which we haven't done yet) before we enable it by default.

[calda]

A good step would be to update our snapshot tests to use the new parsing implementation and make sure these pass. Would you be interested in making this change?

[marcelofabri]

1. We have this test that checks parsing is the same for both strategies. This should be enough, right? We don't need snapshot tests for it. I've also wrote a similar test in Robinhood's codebase to make sure the models were the same.

2. I agree, but also don't think we'll get them without changing the default. Do you think you can give this a try in Airbnb's app before 4.0? I feel like 4.0 should be when we make this change, since it'll require people to manually check their animations anyway (given the rendering engine being switched). Doing those migrations is quite painful (talking from the 3.x migration we just did earlier in the year), so combining breaks changes makes it easier for consumers.

[calda]

Last time I looked at enabling .dictionaryBased by default, I think I got a fair number of test failures. We'd need to update the snapshot tests to cover this first.

I double checked and this is not actually the case -- the snapshot tests pass if we update them from .codable to .dictionaryBased.

Do you think you can give this a try in Airbnb's app before 4.0?

Sure, I'll do this today.

Given the tests actually do pass with this change, I think it makes sense to update this in Lottie 4.0. Would you like to post the PR for this?

[marcelofabri]

yeah, I can put up a PR later today 🙏

[calda]

Confirmed that using .dictionaryBased doesn't cause visual differences in any of the Lottie animations bundled in Airbnb's iOS app 👍🏻

[marcelofabri]

Done in #1777