Addng WriteInstance into LocalInstance inheritance hierarchy #3325

black32167 · 2022-02-12T10:58:00Z

Description

This PR implements #2991.

Test Plan

sc=require("{SC_REPO}/packages/sourcecred/dist/server/api.js")
i = sc.sourcecred.instance.writeInstance.getOriginWriteInstance("{SC_INSTANCE}")
i.writeLedger(new sc.sourcecred.ledger.ledger.Ledger())

…ry methods

black32167 · 2022-02-13T12:29:07Z

Some side notes (not necessarily need be addressed in this PR):

createPluginDirectory method in writeInstance copied one from readInstance, except additionally it was creating the target directory, which feels like a 'storage' responsibility. So just removed createPluginDirectory it from WriteInstance.
Noted that there is a ReadOnlyInstance interface but no ReadWriteInstance interface. Is it feasible to introduce one (unsure)?
Selection of the serialization strategy does not feel as a responsibility of the *Instance. Maybe better to move this logic into something like StrategicStorage wrapper.

blueridger

AMAZING! This is one of the most exciting contributions from a casual contributor that I've seen. Just a few minor comments and updated test plan recommendation:

on second thought, the new storage methods are experimental and we can rigorously test them later. The really important manual testing to do is regression testing for local instance. Here are the steps:

Clone sourcecred/cred and checkout the gh-pages branch.
yarn build in sourcecred/sourcecred
scdev credrank in sourcecred/cred and verify no errors
if you haven't set up the scdev alias, it represents node /Users/thena/workspace/sourcecred/packages/sourcecred/bin/sourcecred.js

packages/sourcecred/src/api/instance/writeInstance.js

Co-authored-by: Thena Seer <seerthena@gmail.com>

black32167 · 2022-02-15T10:42:47Z

packages/sourcecred/src/api/lib/node.js

@@ -14,6 +14,12 @@ const api: any = {...cloneDeep(base)};
 import {LocalInstance} from "../../api/instance/localInstance";
 api.instance.LocalInstance = LocalInstance;

+import {getOriginWriteInstance} from "../../api/instance/writeInstance";


I've added factory methods here because when I tried to register writeInstance module in packages/sourcecred/src/api/lib/base.js, the build was failing with

sourcecred: [webpack-cli] ModuleNotFoundError: Module not found: Error: Can't resolve 'fs' in '/storage/projects/sourcecred/sourcecred-sources/node_modules/better-sqlite3/lib' sourcecred: at /storage/projects/sourcecred/sourcecred-sources/node_modules/webpack/lib/Compilation.js:925:10 sourcecred: at /storage/projects/sourcecred/sourcecred-sources/node_modules/webpack/lib/NormalModuleFactory.js:401:22 sourcecred: at /storage/projects/sourcecred/sourcecred-sources/node_modules/webpack/lib/NormalModuleFactory.js:130:21 sourcecred: at /storage/projects/sourcecred/sourcecred-sources/node_modules/webpack/lib/NormalModuleFactory.js:224:22 sourcecred: at /storage/projects/sourcecred/sourcecred-sources/node_modules/neo-async/async.js:2830:7 sourcecred: at /storage/projects/sourcecred/sourcecred-sources/node_modules/neo-async/async.js:6877:13 sourcecred: at /storage/projects/sourcecred/sourcecred-sources/node_modules/webpack/lib/NormalModuleFactory.js:214:25 sourcecred: at /storage/projects/sourcecred/sourcecred-sources/node_modules/enhanced-resolve/lib/Resolver.js:213:14 sourcecred: at /storage/projects/sourcecred/sourcecred-sources/node_modules/enhanced-resolve/lib/Resolver.js:285:5 sourcecred: at eval (eval at create (/storage/projects/sourcecred/sourcecred-sources/node_modules/tapable/lib/HookCodeFactory.js:33:10), <anonymous>:15:1) sourcecred: at /storage/projects/sourcecred/sourcecred-sources/node_modules/enhanced-resolve/lib/UnsafeCachePlugin.js:44:7 sourcecred: at /storage/projects/sourcecred/sourcecred-sources/node_modules/enhanced-resolve/lib/Resolver.js:285:5 sourcecred: at eval (eval at create (/storage/projects/sourcecred/sourcecred-sources/node_modules/tapable/lib/HookCodeFactory.js:33:10), <anonymous>:15:1) sourcecred: at /storage/projects/sourcecred/sourcecred-sources/node_modules/enhanced-resolve/lib/Resolver.js:285:5 sourcecred: at eval (eval at create (/storage/projects/sourcecred/sourcecred-sources/node_modules/tapable/lib/HookCodeFactory.js:33:10), <anonymous>:27:1) sourcecred: at /storage/projects/sourcecred/sourcecred-sources/node_modules/enhanced-resolve/lib/DescriptionFilePlugin.js:67:43 sourcecred: resolve 'fs' in '/storage/projects/sourcecred/sourcecred-sources/node_modules/better-sqlite3/lib' sourcecred: Parsed request is a module sourcecred: using description file: /storage/projects/sourcecred/sourcecred-sources/node_modules/better-sqlite3/package.json (relative path: ./lib) sourcecred: Field 'browser' doesn't contain a valid alias configuration sourcecred: resolve as module sourcecred: /storage/projects/sourcecred/sourcecred-sources/node_modules/better-sqlite3/lib/node_modules doesn't exist or is not a directory sourcecred: /storage/projects/sourcecred/sourcecred-sources/node_modules/node_modules doesn't exist or is not a directory sourcecred: /storage/projects/sourcecred/node_modules doesn't exist or is not a directory sourcecred: /storage/projects/node_modules doesn't exist or is not a directory sourcecred: /storage/node_modules doesn't exist or is not a directory sourcecred: /node_modules doesn't exist or is not a directory ...

presumably because of specificity in the webpack.config.api.js configuration for base.js. Unsure if that is right or wrong, just noting.

see other comment for solution. it is important that write instance is imported into base.js

black32167 · 2022-02-15T10:49:27Z

The manual testing is done, the output of the scdev credrank (run in the 'sourcecred/cred' repo, 'gh-pages' branch) was looking like:

  GO   credrank
  GO   load data
 DONE  load data: 2005ms
  GO   run CredRank
 DONE  run CredRank: 1m 6s
# Top Participants By Cred

| Description | Cred | % |
| --- | --- | --- |
### personal data ###
...
  GO   writing changes
 DONE  writing changes: 7400ms
 DONE  credrank: 1m 20s

blueridger

Sorry, I wrote these comments and then forget to hit send!

blueridger · 2022-02-16T18:02:00Z

packages/sourcecred/src/api/instance/writeInstance.js

+  //  Private Functions
+  //////////////////////////////
+
+  async readInstanceConfig(): Promise<InstanceConfig> {


This is the method blocking this file from being used in lib/base.js and here is the solution:

move readInstanceConfig and imports from "../instanceConfig" to LocalInstance

move the implementations of readContributionsInput and readGraphInput to the LocalInstance

stub readContributionsInput and readGraphInput in WriteInstance with throw new Error("not implemented")

blueridger · 2022-02-16T18:02:32Z

packages/sourcecred/src/api/lib/node.js

@@ -14,6 +14,12 @@ const api: any = {...cloneDeep(base)};
 import {LocalInstance} from "../../api/instance/localInstance";
 api.instance.LocalInstance = LocalInstance;

+import {getOriginWriteInstance} from "../../api/instance/writeInstance";


see other comment for solution. it is important that write instance is imported into base.js

…Instance-extends-readInstance

…calInstance

This reverts commit a244396.

black32167 · 2022-03-01T12:49:39Z

Applied solution, just one question:

stub readContributionsInput and readGraphInput in WriteInstance with throw new Error("not implemented")

Do we need this step given that WriteInstance extends ReadInstance where these methods are already declared?

blueridger · 2022-03-03T18:22:54Z

Ah good point! No, they wouldn't need stubbed again then.

black32167 · 2022-03-12T06:45:19Z

@blueridger is anything else needed to be done here?

blueridger · 2022-03-14T16:21:49Z

Great work, this is so exciting!

black32167 added 4 commits February 12, 2022 21:55

Created WriteInstance

fa14152

Made LocalInstance extend WriteInstance

626a264

Merged with main

304be7c

preffified

59b7fa4

black32167 requested a review from blueridger February 12, 2022 12:37

Removed baseDirectory constructor param and added WriteInstance facto…

6d6c083

…ry methods

blueridger reviewed Feb 14, 2022

View reviewed changes

packages/sourcecred/src/api/instance/writeInstance.js Outdated Show resolved Hide resolved

packages/sourcecred/src/api/instance/writeInstance.js Outdated Show resolved Hide resolved

black32167 and others added 3 commits February 15, 2022 14:27

Removed rogue _initInstance method

8ba1bfa

Update packages/sourcecred/src/api/instance/writeInstance.js

0e79478

Co-authored-by: Thena Seer <seerthena@gmail.com>

Added WriteInstance factory methods into node.js

a244396

black32167 commented Feb 15, 2022

View reviewed changes

black32167 marked this pull request as ready for review February 15, 2022 10:43

black32167 requested review from blueridger and magwalk February 15, 2022 10:49

blueridger reviewed Feb 28, 2022

View reviewed changes

black32167 added 6 commits March 1, 2022 17:31

Merge remote-tracking branch 'origin/main' into black32167/2991-write…

a0c06e9

…Instance-extends-readInstance

Moved readContributionsInput, readGraphInput,readInstanceConfig to Lo…

8534681

…calInstance

Added WriteInstance factory methods into base.js

2685bce

Revert "Added WriteInstance factory methods into node.js"

ea2d880

This reverts commit a244396.

prettify

477894c

removed redundant overrides from writeInstance

8cc65fb

black32167 requested a review from blueridger March 1, 2022 12:50

blueridger approved these changes Mar 14, 2022

View reviewed changes

blueridger merged commit 0506e48 into main Mar 14, 2022

blueridger deleted the black32167/2991-writeInstance-extends-readInstance branch March 14, 2022 16:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Addng WriteInstance into LocalInstance inheritance hierarchy #3325

Addng WriteInstance into LocalInstance inheritance hierarchy #3325

black32167 commented Feb 12, 2022 •

edited

black32167 commented Feb 13, 2022 •

edited

blueridger left a comment

black32167 Feb 15, 2022

blueridger Feb 16, 2022

black32167 commented Feb 15, 2022 •

edited

blueridger left a comment

blueridger Feb 16, 2022

blueridger Feb 16, 2022

black32167 commented Mar 1, 2022 •

edited

blueridger commented Mar 3, 2022

black32167 commented Mar 12, 2022

blueridger commented Mar 14, 2022

Addng WriteInstance into LocalInstance inheritance hierarchy #3325

Addng WriteInstance into LocalInstance inheritance hierarchy #3325

Conversation

black32167 commented Feb 12, 2022 • edited

Description

Test Plan

black32167 commented Feb 13, 2022 • edited

blueridger left a comment

Choose a reason for hiding this comment

black32167 Feb 15, 2022

Choose a reason for hiding this comment

blueridger Feb 16, 2022

Choose a reason for hiding this comment

black32167 commented Feb 15, 2022 • edited

blueridger left a comment

Choose a reason for hiding this comment

blueridger Feb 16, 2022

Choose a reason for hiding this comment

blueridger Feb 16, 2022

Choose a reason for hiding this comment

black32167 commented Mar 1, 2022 • edited

blueridger commented Mar 3, 2022

black32167 commented Mar 12, 2022

blueridger commented Mar 14, 2022

black32167 commented Feb 12, 2022 •

edited

black32167 commented Feb 13, 2022 •

edited

black32167 commented Feb 15, 2022 •

edited

black32167 commented Mar 1, 2022 •

edited