Details

Reviewers

michal
tomek

Commits

rCOMM4ec416f4717d: [web-db] add database module provider

Summary

Making database connection accessible from entire web app

Depends on D7188

Test Plan

Check how it behaves in multiple places in web app, try sending some worker messages, e.g.:

databaseModule
  .schedule({
    type: workerRequestMessageTypes.GET_CLIENT_STORE,
  })
  .then(e => {
    console.log(e.store.drafts);
  })
  .catch(e => {
    console.error(e);
  });

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

kamil created this revision.Mar 27 2023, 6:24 AM

kamil held this revision as a draft.

Herald added subscribers: atul, ashoat. · View Herald TranscriptMar 27 2023, 6:24 AM

kamil added a child revision: D7190: [web-db] implement custom `redux-persist` storage.Mar 27 2023, 6:27 AM

Harbormaster failed remote builds in B17663: Diff 24183!Mar 27 2023, 6:41 AM

rebase

Harbormaster completed remote builds in B17685: Diff 24208.Mar 27 2023, 8:20 AM

kamil published this revision for review.Mar 27 2023, 8:30 AM

ashoat added inline comments.Mar 27 2023, 4:11 PM

web/database/database-module-provider.js

43–57 ↗

(On Diff #24208)

You can await anywhere actually:

(async () => {
  try {
    await this.workerProxy.scheduleOnWorker({
      type: workerRequestMessageTypes.INIT,
      sqljsFilePath: `${origin}${SQLJS_FILE_PATH}`,
      sqljsFilename,
    });
    this.status = databaseStatuses.initSuccess;
    console.info('Database initialization success');
  } catch (error) {
    this.status = databaseStatuses.initError;
    console.error(`Database initialization failure`, error);
  }
})();

tomek requested changes to this revision.Mar 30 2023, 4:54 AM

tomek added inline comments.

web/database/database-module-provider.js
5–9 ↗	(On Diff #24208)	We can merge these
11–12 ↗	(On Diff #24208)	Why do we have to do this?
33–35 ↗	(On Diff #24208)	Do we have to set these when the db is not supported?
39 ↗	(On Diff #24208)	This overrides `databaseStatuses.notSupported` status - is that intentional?
72 ↗	(On Diff #24208)	Are we sure about this? When the promise is resolved the caller might think that the operation was successful while we haven't scheduled it at all

This revision now requires changes to proceed.Mar 30 2023, 4:54 AM

refactor code

@tomek I changed the logic a bit, so I will not respond directly to your questions but adding inline comments which should cover are your concerns

web/database/database-module-provider.js
14 ↗	(On Diff #24553)	when the user will open a web or desktop app and is already logged in we need to know this before redux store is rehydrated (`redux-persist` will be in SQLite db in the future) - that being said we need to rely on `preloadedState` because that's the case when we will read from the database
16–21 ↗	(On Diff #24553)	`notSupported` - database is not supported, the user is not logged in or is a non-staff user `initSuccess` - initialization ended with success and the database can be used `initInProgress`- initialization promise was started `initError` - there was an error and the database is not initialized and can not be used
59–72 ↗	(On Diff #24553)	object of this class is created here in this file and then exported, promises could not be awaited in the top module so in we only start an init promise
75–82 ↗	(On Diff #24553)	database is not supported when there is no user, when user will log in we need to initialize the database for him
91–93 ↗	(On Diff #24553)	we try to schedule db operation but first we need to await the init promise to have a fully initialized database