Page MenuHomePhabricator

[web] Initialize database at startup on safari
ClosedPublic

Authored by michal on Sep 8 2023, 5:31 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Jun 22, 8:42 PM
Unknown Object (File)
Sat, Jun 15, 9:11 AM
Unknown Object (File)
Fri, Jun 14, 1:05 PM
Unknown Object (File)
Thu, Jun 13, 10:05 AM
Unknown Object (File)
Tue, Jun 11, 3:25 PM
Unknown Object (File)
Mon, Jun 10, 9:29 AM
Unknown Object (File)
Mon, Jun 10, 9:29 AM
Unknown Object (File)
Fri, May 31, 7:03 PM
Subscribers

Details

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

Small fixed, update test plan

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

michal requested review of this revision.Sep 8 2023, 6:07 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
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

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

Set DatabaseModule fields to optional and add invariants