Ws sync

[RangerMauve]

For mapeo-web websocket sync

[hackergrrl]

Cool :fire:

fyi we aren't using package-lock on our modules (just top-level app repos like mapeo-{desktop,mobile}).

[okdistribute]

👋 happy to see this progress @RangerMauve! We do have package-lock in mapeo-core (begrudgingly perhaps) because we were seeing discrepancies at some point and it made it difficult to know when breaking changes were happening during tests (to keep CI green!) but in /theory/ it should be OK to have package-locks only in the apps; many other projects do that. 🤷

FYI there's a PR here #107 that might conflict with this one -- it includes some inline comments for the sync.js file, which is quite large, and contains lots of workaround logic (cough, hacks) as we ran into issues during real world testing. Some of those issues can't be really fixed (the right way) until sparse replication is possible (whew)!

Anyway, hope you don't mind me chiming in -- wonderful work!!

[RangerMauve]

I'm not sure how the testing should work. Should I embed mapeo-web? Or should I create a mini websocket server for testing the replication without the rest of mapeo-web?

[gmaclennan]

I'm not sure how the testing should work. Should I embed mapeo-web? Or should I create a mini websocket server for testing the replication without the rest of mapeo-web?

I think the goal of tests for this part of the code is to ensure that if someone changes the sync code, then we know that mapeo-core can still sync with mapeo web, e.g. we want to ensure that features or bug fixes here do not break web syncing.

I think embedding mapeo-web seems the most reliable way to do this. A mini websocket server seems a bit like mocking, and could pass or fail because the mock is different to the actual server. I think we could/should also put this kind of end-to-end test in Mapeo (Mobile|Desktop) but at this time I think this is the best place for it.

[RangerMauve]

Right now the tests are inside mapeo-web since it has mapeo-core as a dependency. Do we want both of them to be testing each other?

[RangerMauve]

Hey, just wanted to get feedback on what we need before merging.

I think there was the blob size thing from Sabella's design, but I'm not sure if that can just come in a separate PR.

[RangerMauve]

Sweet, thanks. What should the tests look like? Should I add mapeo-web as a dev-dependency and set up a local instance to sync with?

[gmaclennan]

Sweet, thanks. What should the tests look like? Should I add mapeo-web as a dev-dependency and set up a local instance to sync with?

Yes, I think similar to the mapeo-web tests. Maybe could be the same tests? The tests should check that the functionality you have added to the sync methods actually works as stated. Speaking of which, we also need documentation in the README of how to use the API. The tests should ensure that the documentation is correct (e.g. if the method is documented as "sync with a Mapeo Cloud instance over websocket", then we should test this is what the method does.

[RangerMauve]

@gmaclennan Added tests and docs. mapeo-web should be released before we merge.

[gmaclennan]

@rangermauve the tests are failing in Node v12.20.0 on all platforms. I'm just re-running the tests on a few other node versions to figure out if this is specific to that Node version or not. https://github.com/digidem/mapeo-core/actions/runs/512282549

[gmaclennan]

Seems like this fails from Node v12.19.1, v12.20.0, v12.20.1. Works prior to v12.19.1 and works in v14.

Also, tests seem to be randomly hanging at a different point on Windows. @noffle have you seen this before? I don't think these are related to this PR, but perhaps due to how deps have changed (I think Node was on v12.18 last time we did a release)

[hackergrrl]

I ran a git bisect with node v12.20.1 and it looks like the new mapeo-web test added in a49798e7205242edb57af9733a3d515a360317e2 introduced the crash.

I poked at it a bit, and it seems to be timing related. I wrote a repro case in 87d4650bcecc0a818699e908cbc5da17112634da. It runs the mapeo-web test, then sleeps for 80s. When it starts the next test that uses a mapeo-core instance, the leveldb crash is triggered. This could just be a weird localized node version issue, or could hint at a nastier bug that might show up for us later.

[gmaclennan]

Copying across what I wrote in chat. I see two separate issues coming up here:

1. A leveldb failure on Node >=12.19.1
2. A sync timeout on Windows on some Node versions (seems random at the moment) My take is that both of these are existing bugs unrelated to Mauve's new code, but they have been surfaced by new tests. Mapeo Desktop is currently using Electron v9.2.1, which uses Node v12.14.1, which is not showing the issue (1) in the tests. Mapeo Mobile is using nodejs-mobile-react-native v0.6.1, which uses nodejs-mobile v0.3.1, which uses Node v12.16.3, which also passes tests I think. However, we should be wary about the newer nodejs-mobile-react-native v0.6.2 which uses Node v12.19.0 on nodejs-mobile v0.3.2. So, on issue (1) I think we are ok, since the Node versions this will run on do not exhibit the issue. The Windows issue (2) seems to be separate (it affects other Node versions), and it is an issue that our users have experienced in the field (sync on Windows getting stuck), so I think this PR / test is merely catching an existing issue, that we should fix. TL;DR I think the solution is to ignore these failing tests and merge. @noffle what is your take on how to move forwards here?

I have updated the Node versions used in these tests here to target the actual versions of Node that we ship with Mapeo Mobile and Mapeo Desktop, along with the Node versions that we would use if we update electron / nodejs-mobile to latest. I think we can push dealing with the Node >= 12.19.0 issue for now.

[RangerMauve]

Yay!