peer-base / js-delta-crdts

Delta State-based CRDTs in Javascript
194 stars 16 forks source link

Workaround for s.add, s.removeValue function resolution #44

Closed jayaddison closed 5 years ago

jayaddison commented 5 years ago

Hi @jimpick @pgte - first of all, thanks for the peer-base and js-delta-crdts codebases; these are great. I've been enjoying building with them and learning about the technologies involved.

I'm writing a browser-based application and would like to use a set with aworset semantics; add-wins seems most appropriate.

To include the dependencies, the application uses browserify@16.5.0 executed via browserify --standalone peer-base node_modules/peer-base/src/index.js. The resulting output script is included via a script tag, exposing peer-base functionality at window.peerBase in the DOM.

The first time the page loads - in either Firefox or Chrome - it's possible to add and remove items to the set:

# Firefox 69.0.1

var v, app = window.peerBase('app');
app.start().then(async function() {
    v = await app.collaborate('collaboration-name-1d7fa8', 'aworset');
});
> Promise { <state>: "pending" }
> Swarm listening on ...
> No options.keys ...

v.shared.value()
> Set []

v.shared.add('test')
> Promise { <state>: "fulfilled", <value>: undefined }

v.shared.add('test2')
> Promise { <state>: "fulfilled", <value>: undefined }

v.shared.remove('test')
> Promise { <state>: "fulfilled", <value>: undefined }

v.shared.value()
> Set [ "test2" ]

However when the page is loaded a second time - despite the v.shared.value being reloaded correctly - it becomes impossible to perform set operations:

# Firefox 69.0.1

v.shared.value()
> Set [ "test2" ]

v.shared.add('test3')
> Promise { <state>: "rejected" }
> TypeError: v.removeValue is not a function

v.shared.remove('test2')
> Promise { <state>: "rejected" }
> TypeError: v.removeValue is not a function

After some runtime debugging I found that the object s at these two locations:

https://github.com/ipfs-shipyard/js-delta-crdts/blob/0eb76c511952883f816eed3995d2963a76483343/src/aworset.js#L10 https://github.com/ipfs-shipyard/js-delta-crdts/blob/0eb76c511952883f816eed3995d2963a76483343/src/aworset.js#L13

... is a raw JSON dictionary during the second page load, instead of an expected instance of type DotSet.

This pull request has a workaround - it doesn't feel like it's the correct solution, but I'm opening it to support the description of the problem and highlight what's going wrong. Glad for any pointers and to help resolve this correctly.

jayaddison commented 5 years ago

NB: I'll try to add a test case soon to expose/reproduce this issue

jayaddison commented 5 years ago

Digging a little further, the issue is caused by a type expectation mismatch between peer-base and js-delta-crdts in calls made here: https://github.com/peer-base/peer-base/blob/35166484f71e67c668f39982a8333099f2fd1b9e/src/collaboration/shared.js#L60

peer-base is sending the state value (which becomes the s arg) as a Javascript object (not a JSON object as I mistakenly said previously), whereas js-delta-crdts expects s to be a native CRDT type.

Looking into some potential fixes here. The easiest might be to call crdtType.value(state) from peer-base to load the state into a new type instance, but perhaps there's a cached value available to prevent all the work of creating a new type for each mutation call.

jayaddison commented 5 years ago

The correct fix here is to make sure that crtdType.join has been called after initial state is loaded from the IPFS store; this ensures that the state value constructed by peer-base is a first-class CRDT instance rather than a raw object.

This hsa already been fixed in https://github.com/peer-base/peer-base/pull/283 - I was using an older version of peer-base (0.12.0-rc6). Have now confirmed that pulling in this change resolves the issue.

jimpick commented 5 years ago

Thanks for the PR and trying out peer-base!

jayaddison commented 5 years ago

Thanks @jimpick ! Is there any chance the version of peer-base in npm could be bumped up to include the fix in master?

jimpick commented 5 years ago

That would be handy for me too. @pgte can you do a js-delta-crdts release?

pgte commented 5 years ago

@jimpick done, released v0.10.3.

jayaddison commented 5 years ago

Awesome, thanks both of you!