Page MenuHomePhabricator

[web-db] add database module provider
ClosedPublic

Authored by kamil on Mar 27 2023, 6:24 AM.
Tags
None
Referenced Files
F2025659: D7189.id24208.diff
Mon, Jun 17, 11:44 AM
Unknown Object (File)
Fri, Jun 14, 6:02 PM
Unknown Object (File)
Tue, Jun 11, 4:31 AM
Unknown Object (File)
Thu, May 30, 5:32 AM
Unknown Object (File)
Thu, May 30, 5:32 AM
Unknown Object (File)
Sun, May 26, 4:47 PM
Unknown Object (File)
Sun, May 26, 3:35 AM
Unknown Object (File)
Sun, May 26, 3:35 AM
Subscribers

Details

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
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

kamil held this revision as a draft.
kamil published this revision for review.Mar 27 2023, 8:30 AM
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

@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

tomek added inline comments.
web/database/database-module-provider.js
44–51 ↗(On Diff #24553)

Can we simplify this?

This revision is now accepted and ready to land.Apr 4 2023, 1:46 AM
This revision was automatically updated to reflect the committed changes.