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

Fix late initialization error for Analytics.userProperty #239

Merged
merged 5 commits into from
Feb 28, 2024

Conversation

eliasyishak
Copy link
Contributor

Fixes:

TLDR: initialization logic within the Session class's constructor body expects Analytics.userProperty to be initialized, this moves that constructor body logic from Session into a separate method (Session.initialize) we will call at the end of the Analytics class's constructor after we are sure all necessary fields have been initialized


  • 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.

Copy link

Package publishing

Package Version Status Publish tag (post-merge)
package:unified_analytics 5.8.5 ready to publish unified_analytics-v5.8.5
package:cli_config 0.1.2 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.

@eliasyishak eliasyishak changed the title Fix late initialization error Fix late initialization error for Analytics.userProperty Feb 27, 2024
/// because the refresh method will write to the session file and for
/// users that have opted out, we have to leave the session file empty
/// per the privacy document
void initialize(bool telemetryEnabled) {
Copy link
Member

Choose a reason for hiding this comment

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

it worries me anytime there is an initialize() function that one must remember to call at the right time, because a refactor could easily lead to code now accessing state in this class BEFORE the initialize() method was called.

I think this would solve the particular issue linked, and maybe this is the right change to make immediately to stop crashes, but I think long-term we should refactor all of these classes (especially Analytics) such that it is not possible to access any of its state BEFORE it has been initialized.

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.

This LGTM as a fix to get to users.

As discussed, I think ideally we would make all class fields either final or late final with initializer expressions, to ensure we don't hit LateInitializationErrors.

@eliasyishak
Copy link
Contributor Author

Going to merge this in now and publish so we can have a fix, filed a separate issue to refactor session handler

@eliasyishak eliasyishak merged commit fca993e into dart-lang:main Feb 28, 2024
6 checks passed
@eliasyishak eliasyishak deleted the fix-late-initialization-error branch February 28, 2024 18:01
@christopherfujino
Copy link
Member

christopherfujino commented Feb 28, 2024

Going to merge this in now and publish so we can have a fix, filed a separate issue to refactor session handler

Can we also track refactoring all the late/late final fields to something safer? I feel like unless one can reason globally about all the state in this package and all possible edge cases, it's impossible to know that users won't hit another LateInitializationError.

@eliasyishak
Copy link
Contributor Author

Definitely, I captured here: #241

copybara-service bot pushed a commit to dart-lang/sdk that referenced this pull request Feb 28, 2024
Revisions updated by `dart tools/rev_sdk_deps.dart`.

dartdoc (https://github.com/dart-lang/dartdoc/compare/40c470e..eed92d3):
  eed92d3f  2024-02-28  Sam Rawlins  Simplify InheritingContainer.allModelElements and _inheritedElements (dart-lang/dartdoc#3689)
  7b3274a6  2024-02-28  Sam Rawlins  Do not require FLUTTER_ROOT any longer, even for flutter packages (dart-lang/dartdoc#3688)
  d25dfca4  2024-02-27  Kallen Tu  Remove LanguageFeatureRenderer. (dart-lang/dartdoc#3686)
  d8e7f99c  2024-02-27  Sam Rawlins  Display the immediate representation type, not any erasure (dart-lang/dartdoc#3685)
  91c361af  2024-02-27  Kallen Tu  Remove SourceCodeRenderer and TypeParametersRenderer. (dart-lang/dartdoc#3683)

markdown (https://github.com/dart-lang/markdown/compare/d735b0b..62e3349):
  62e3349  2024-02-27  Tom Yeh  Fix `#586`: encode image tag's src attribute (dart-lang/markdown#589)

mockito (https://github.com/dart-lang/mockito/compare/7d6632f..3ef744f):
  3ef744f  2024-02-27  Ilya Yanok  Bump SDK version using in CI to 3.3
  ecec7c1  2024-02-01  dependabot[bot]  Bump dart-lang/setup-dart from 1.6.0 to 1.6.2
  b693ada  2024-02-27  Ilya Yanok  Add basic extension types support

test (https://github.com/dart-lang/test/compare/a3f05ec..26953ba):
  26953ba4  2024-02-26  Sam Rawlins  Update README.md, remove link to Stream Matchers (dart-lang/test#2187)

tools (https://github.com/dart-lang/tools/compare/c7fbf26..fca993e):
  fca993e  2024-02-28  Elias Yishak  Fix late initialization error for `Analytics.userProperty` (dart-lang/tools#239)
  1a0d7da  2024-02-26  Elias Yishak  Export `testing.dart` with all enums (dart-lang/tools#237)

web (https://github.com/dart-lang/web/compare/d96c01d..fa4280c):
  fa4280c  2024-02-27  Devon Carew  mention the migration guide in the readme (dart-lang/web#187)

Change-Id: Ib183b4af55146fdeabfb18dc91bf642159e67a1b
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/354963
Reviewed-by: Kevin Moore <kevmoo@google.com>
Commit-Queue: Devon Carew <devoncarew@google.com>
@christopherfujino
Copy link
Member

Definitely, I captured here: #241

nice, thanks!

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.

2 participants