Introduction of a SuperThis #183

mabels · 2024-08-19T08:37:08Z

This should change no behavior but finally bring a stable dependency injection platform to the codebase.

mschoch · 2024-08-20T18:25:04Z

This is a pretty wide change and I would benefit from some additional context to properly review.

What is a SuperThis? Is this a common construct in the js/ts world for dependency injection purposes?

mabels · 2024-08-21T12:32:06Z

The superthis is the finalization of the passthrough of the logger

--- but due to the endless loop fix I ran into the situation that the "async" process doesn't enough about the "runtime" and I'm not able to pass the runtime down.
I decided to go for the ugliest but only working pattern to do "dependency inject" pass the runtime in a single-process execution model.
And this is to add an explicit parameter passed through all layers. I named it superthis because it is the second parameter beside the first "this/self" --- in other runtimes, it is named "context" which you have to pass and with is not typed.

mschoch · 2024-08-21T13:43:19Z

At least we agree that it is ugly.

mschoch · 2024-08-21T21:51:10Z

OK, so since my ability to review this in the abstract is somewhat limited. I instead set out to test this branch with the gateway I've been working on. It took some effort to even get things running as well as before, but it was useful in at least letting me appreciate these changes in practice (most of the questions I had could be answered by the PR itself).

Also, I've heavily modified the PK logic to function almost identically to the file gateway now (just writes to the party room instead of disk). Eventually we'll make use of the websocket again for the realtime updates.

As I said, my test still doesn't work. It doesn't hang, Bob just never sees the data that Alice wrote (syncing to same PK).

One question I have, in the test, I try to force the update with (shown are my previous and current way of doing it)

- await bob.blockstore.loader?.remoteMetaStore?.load('main')
+ await connectionBob.loader?.remoteMetaStore?.load('main')

Is that correct?

mschoch

This is a first-pass review, with some notes intended to help @necrodome join the conversation.

SuperThis is somewhat equivalent to a context object, in Go context.Context is a reasonable analog
Existing 'SuperThis' name is problematic, alternate under consideration "fpContext"
As a coding convention, we would standarize on this as the first argument when wiring up the codebase
Previously we passed a logger instance to most places, and now the logger is inside sthis, so many of the changes in this PR are of the form s/logger/sthis/
I can report that this branch basically works (last tested by me before all commits were present)

My open questions/concerns

It would be great to remove the extraneous stuff from this PR, specifically fragmentation stuff is unrelated
What is the long-term plan for RemoteMetaStore (lots of refs just commented out)
Is there any cleaner/standard way of doing this is JS/TS world? Are there are libraries that do something similar that we can point to?
I would like to clarify the life-cycle of SuperThis. In Go in particular, you are advised to not store context objects inside structs, and I believe the context objects are themselves immutable (modifying it creates a new instance). This detail will become important if we are using it for things like instrumentation/tracing which may contain state (ie, current active span). I ask this because I see mostly pass sthis during initialization and stash it inside the struct. This may also relate to the clone() method, would like it's usage clarified.
I interpretted jchris's guidance to favor moving quickly at the moment, so I also want to be cognizant of overdoing this review

mschoch · 2024-08-28T17:32:25Z

Alright I thought maybe I could fix up some minor things and get it usable again, but seems to be more significant than I realized.

mschoch · 2024-08-28T17:38:32Z

For the time being I'm basing future work on eb8b739 as that was the last reliable SHA for me on this branch. And I was expecting we'd get agreement on something close to that merged. There's going to be some rework to pull all this together no matter which way we go.

jchris · 2024-08-30T14:50:41Z

src/runtime/gateways/file/utils.ts

  }
-  return SysContainer.join(basePath);


this looks like the action

jchris · 2024-08-31T15:54:36Z

for integration work with connect repo I am using 22d9221 for now

jchris · 2024-09-02T19:52:34Z

is this part of #185 now? I think we should merge both

tests/fireproof/crdt.test.ts

tests/fireproof/database.test.ts

chore: replace sys -> pathOps chore: added CarTransactionOpts chore: remove uuid chore: integrated registerKeyBagProviderFactory chore: enable stack on log chore: introduce memorygateway

src/blockstore/meta-key-helper.ts

src/blockstore/store-remote.ts

src/crdt.ts

jchris · 2024-10-10T14:28:49Z

src/database.ts

+  );
+}
+
+export class DatabaseShell<DT extends DocTypes = NonNullable<unknown>> implements Database<DT> {


I wanna understand what motivates this layer

close need to be refcounted
if you open multiple times the same database like:
db1 = fireproof("test")
db2 = fireproof("test")
db3 = fireproof("test")
db4 = fireproof("test")
db1.close()
db2.close()
db4.close()
db3.close() // realclose is done. w
then close should do nothing until the last instance is closed.
We had some test which tested for that behaviour.

src/runtime/gateways/file/node-filesystem.ts

jchris · 2024-10-11T16:25:01Z

src/runtime/gateways/memory/version.ts

@@ -0,0 +1 @@
+export const MEMORY_VERSION = "v0.19-memory";


is this supposed to be checked in?

jchris · 2024-10-11T16:28:36Z

tests/fireproof/fireproof.test.ts

@@ -84,7 +97,7 @@ describe("public API", function () {
  });
  it("should be a database instance", function () {
    expect(db).toBeTruthy();
-    expect(db instanceof Database).toBeTruthy();
+    expect(isDatabase(db)).toBeTruthy();


not sure our downstream users can be expected not to use instanceof maybe we should have both assertions? I'm not sure what isDatabase would be used for outside of this, so maybe we remove the test?

jchris

thanks!

mabels force-pushed the super-this branch from e1d3e02 to b9778a8 Compare August 20, 2024 16:07

mabels requested review from jchris and mschoch August 20, 2024 16:07

mschoch reviewed Aug 26, 2024

View reviewed changes

mschoch requested a review from necrodome August 26, 2024 16:08

jchris reviewed Aug 30, 2024

View reviewed changes

src/runtime/gateways/file/utils.ts Outdated

}

return SysContainer.join(basePath);

Copy link

Contributor

jchris Aug 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks like the action

jchris mentioned this pull request Aug 31, 2024

first pass partykit connector fireproof-storage/connect#7

Closed

mabels force-pushed the super-this branch from 177623f to dfea2c9 Compare September 19, 2024 20:08

jchris reviewed Sep 19, 2024

View reviewed changes

tests/fireproof/crdt.test.ts Outdated Show resolved Hide resolved

jchris reviewed Sep 19, 2024

View reviewed changes

tests/fireproof/database.test.ts Outdated Show resolved Hide resolved

mabels added 2 commits October 9, 2024 16:30

chore: improve the storekey url parameter handling

2b5ea37

chore: replace sys -> pathOps chore: added CarTransactionOpts chore: remove uuid chore: integrated registerKeyBagProviderFactory chore: enable stack on log chore: introduce memorygateway

chore: make the tests working

df86075

mabels force-pushed the super-this branch from b15434e to df86075 Compare October 10, 2024 07:42

chore: fix deno test errors

c73c4fb

mabels self-assigned this Oct 10, 2024

mabels marked this pull request as ready for review October 10, 2024 11:04

jchris reviewed Oct 10, 2024

View reviewed changes

src/blockstore/meta-key-helper.ts Show resolved Hide resolved

jchris reviewed Oct 10, 2024

View reviewed changes

src/blockstore/store-remote.ts Show resolved Hide resolved

jchris reviewed Oct 10, 2024

View reviewed changes

src/crdt.ts Show resolved Hide resolved

jchris reviewed Oct 10, 2024

View reviewed changes

src/runtime/gateways/file/node-filesystem.ts Show resolved Hide resolved

jchris reviewed Oct 11, 2024

View reviewed changes

jchris approved these changes Oct 11, 2024

View reviewed changes

jchris merged commit 81b8822 into main Oct 11, 2024
2 checks passed

mabels deleted the super-this branch October 16, 2024 04:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduction of a SuperThis #183

Introduction of a SuperThis #183

mabels commented Aug 19, 2024

mschoch commented Aug 20, 2024

mabels commented Aug 21, 2024

mschoch commented Aug 21, 2024

mschoch commented Aug 21, 2024

mschoch left a comment

mschoch commented Aug 28, 2024

mschoch commented Aug 28, 2024

jchris Aug 30, 2024

jchris commented Aug 31, 2024

jchris commented Sep 2, 2024

jchris Oct 10, 2024

mabels Oct 10, 2024 •

edited

Loading

jchris Oct 11, 2024

jchris Oct 11, 2024

jchris left a comment

Introduction of a SuperThis #183

Introduction of a SuperThis #183

Conversation

mabels commented Aug 19, 2024

mschoch commented Aug 20, 2024

mabels commented Aug 21, 2024

mschoch commented Aug 21, 2024

mschoch commented Aug 21, 2024

mschoch left a comment

Choose a reason for hiding this comment

My open questions/concerns

mschoch commented Aug 28, 2024

mschoch commented Aug 28, 2024

jchris Aug 30, 2024

Choose a reason for hiding this comment

jchris commented Aug 31, 2024

jchris commented Sep 2, 2024

jchris Oct 10, 2024

Choose a reason for hiding this comment

mabels Oct 10, 2024 • edited Loading

Choose a reason for hiding this comment

jchris Oct 11, 2024

Choose a reason for hiding this comment

jchris Oct 11, 2024

Choose a reason for hiding this comment

jchris left a comment

Choose a reason for hiding this comment

mabels Oct 10, 2024 •

edited

Loading