Skip to content
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

Rewrite the last_ping key again #255

Merged

Conversation

eliasyishak
Copy link
Contributor

@eliasyishak eliasyishak commented Mar 14, 2024

Fixes:

Not having the last_ping key in the session json file will cause issues if a user chooses to downgrade to a previous version of this package that attempts to parse last_ping. This PR adds that value back


  • I’ve reviewed the contributor guide and applied the relevant portions to this PR.
Contribution guidelines:

Note that many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.

Comment on lines 87 to 88
sessionFile.writeAsStringSync(
'{"session_id": ${now.millisecondsSinceEpoch}, "last_ping": null}');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Explanation for why I think this is necessary:

Copy link

Package publishing

Package Version Status Publish tag (post-merge)
package:unified_analytics 5.8.7 ready to publish unified_analytics-v5.8.7
package:cli_config 0.2.0 already published at pub.dev
package:extension_discovery 2.0.1-wip WIP (no publish necessary)
package:graphs 2.3.2-wip WIP (no publish necessary)

Documentation at https://github.com/dart-lang/ecosystem/wiki/Publishing-automation.

@@ -85,7 +85,8 @@ class Initializer {
final now = sessionIdOverride ?? clock.now();
sessionFile.createSync(recursive: true);
sessionFile
.writeAsStringSync('{"session_id": ${now.millisecondsSinceEpoch}}');
.writeAsStringSync('{"session_id": ${now.millisecondsSinceEpoch}, '
'"last_ping": ${now.millisecondsSinceEpoch}}');
Copy link
Member

Choose a reason for hiding this comment

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

can we add a comment here that its deprecated?

// Fallback to setting the session id as the current time
_sessionId = now.millisecondsSinceEpoch;
// ignore: avoid_catching_errors
} on TypeError catch (err) {
Copy link
Member

Choose a reason for hiding this comment

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

let's not catch TypeError, and instead be disciplined about never deleting fields from this JSON file.

Copy link
Member

@christopherfujino christopherfujino left a comment

Choose a reason for hiding this comment

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

LGTM

@eliasyishak eliasyishak merged commit 0a9f87c into dart-lang:main Mar 14, 2024
6 checks passed
@eliasyishak eliasyishak deleted the 252-type-error-on-session-json branch March 14, 2024 21:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants