Details

Reviewers

kamil
atul
inka
tomek
ashoat

Commits

rCOMM5704f7fa8d34: [web] Initialize database at startup on safari

Summary

Part of ENG-4844
Fixed ENG-4878
Depends on D9107

On safari we need to start database before rehydration like on other browsers. This is done by initializing the database in getDatabaseModule, so as soon as it's called somewhere we make sure the database is ready to go. This diff also merges initForDBForLoggedInUser, init, and constructor because there was some code duplication.

Test Plan

Test in safari and in chromium:

check if there's "Database initialization success" when logging in and when reloading when logged in
check if clearSensitiveData is called after logging out
check if draft are still there after reload (drafts are persisted with db operations)
check if selected apps are still there after reload (selected apps are persisted with standard redux persist mechanism)

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

michal created this revision.Sep 8 2023, 5:31 AM

Herald added a subscriber: ashoat. · View Herald TranscriptSep 8 2023, 5:31 AM

michal removed reviewers: kamil, atul, inka, tomek.Sep 8 2023, 5:40 AM

michal edited the test plan for this revision. (Show Details)Sep 8 2023, 5:45 AM

Small fixed, update test plan

Harbormaster completed remote builds in B22442: Diff 30858.Sep 8 2023, 5:49 AM

michal added reviewers: kamil, atul, inka, tomek.Sep 8 2023, 5:50 AM

michal edited the summary of this revision. (Show Details)

michal added inline comments.

web/database/database-module-provider.js
42–44 ↗	(On Diff #30860)	We don't activate db for anonymous (logged out) users

Harbormaster completed remote builds in B22444: Diff 30860.Sep 8 2023, 6:07 AM

michal requested review of this revision.Sep 8 2023, 6:07 AM

michal added a child revision: D9109: [web] Enable db for all users.Sep 8 2023, 6:40 AM

Thanks for this!

web/database/database-module-provider.js
48 ↗	(On Diff #30860)	shouldn't this be `databaseStatuses.notSupported`?

This revision is now accepted and ready to land.Sep 10 2023, 11:50 PM

kamil added inline comments.Sep 11 2023, 12:02 AM

web/database/database-module-provider.js
64–67 ↗	(On Diff #30860)	maybe let's put this code below line 69? ( `this.status = databaseStatuses.initInProgress;`)

Change initError to notSupported, move safari key init after setting status to initInProgress

Harbormaster completed remote builds in B22480: Diff 30911.Sep 11 2023, 12:50 AM

ashoat added inline comments.Sep 12 2023, 10:10 AM

web/database/database-module-provider.js
38 ↗	(On Diff #30911)	Shouldn't this type be `?Promise<void>`? It doesn't seem to be set in the constructor
58–59 ↗	(On Diff #30911)	I'm confused about these lines. Where is `status` defined? Should this be `this.status`? If so, why doesn't Flow complain?

ashoat requested changes to this revision.Sep 12 2023, 11:15 AM

ashoat added inline comments.

web/database/database-module-provider.js
58–59 ↗	(On Diff #30911)	I think Flow doesn't complain because of this line: it looks like `window.status` is a thing, and everything in `window` is in the global namespace as well

This revision now requires changes to proceed.Sep 12 2023, 11:15 AM

Fixed window.status -> this.status issue

web/database/database-module-provider.js
38 ↗	(On Diff #30911)	Flow didn't complain because (flow docs): "However, Flow will not enforce that all class properties are initialized in constructors" Actually all of these fields should be set to possibly undefined if we want to be correct, but that would add some invariants/if checks. I have a solution that is typed correctly and without invariants, but not sure if it's worth it as it's a bit more complex.

I won't block this diff on it, but I think we should strive to have correct types. invariants can feel messy, but I think they are better than having incorrect types

This revision is now accepted and ready to land.Sep 13 2023, 9:44 AM

Harbormaster completed remote builds in B22595: Diff 31065.Sep 13 2023, 9:47 AM

Set DatabaseModule fields to optional and add invariants

Harbormaster completed remote builds in B22656: Diff 31169.Sep 15 2023, 5:55 AM

Nice thank you!!

Closed by commit rCOMM5704f7fa8d34: [web] Initialize database at startup on safari (authored by Michal Gniadek <michal.gniadek@swmansion.com>). · Explain WhySep 18 2023, 3:26 AM

This revision was automatically updated to reflect the committed changes.

Michal Gniadek <michal.gniadek@swmansion.com> added a commit: rCOMM5704f7fa8d34: [web] Initialize database at startup on safari.