Skip to content

Conversation

@szakarias
Copy link
Contributor

No description provided.

Comment on lines +253 to +257
.withCodec(utf8)
.withCodec(json)
.withCodec(wrapAsCodec(
encode: (WeeklyDownloadCounts wdc) => wdc.toJson(),
decode: (d) => WeeklyDownloadCounts.fromJson(d as Map<String, dynamic>),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By default it's possible to store binary data in redis.

That's why we have .withCodec(utf8).

So why not just store:

base64.encode(Uint8List.sublistView(Uint32List.fromList(dataPoints)));

Or:

return Uint8List.sublistView(Uint32List.fromList(dataPoints));

Copy link
Member

@jonasfj jonasfj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'll name to name options for the widget as data-<widget>-<option>.

So if you have data-widget="weekly-sparkline" the points option should be: data-weekly-sparkline-points="...".

}

@JsonSerializable(includeIfNull: false)
class WeeklyDownloadCounts {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not pretty, but you could just pass around the binary encoding 🤣 Or the base64 encoding.

This could also be used in the redis cache. Then you won't have to JSON decode, and reencode as binary for every request.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is indeed an optimization. But as you say it's not pretty. Also the other things we store in redis are human readable strings which is nice for debugging.

@szakarias szakarias merged commit 8a8de69 into dart-lang:master Oct 31, 2024
32 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants