y-crdt / ydotnet

.NET bindings for yrs.
MIT License
35 stars 9 forks source link

Package yrs binaries as Nuget packages #15

Open goldsam opened 1 year ago

goldsam commented 1 year ago

Consumption of this work (upon release) would be easier if the Yrs binaries were provided as nuget packages. If you would like, I could put together a PR for this.

LSViana commented 1 year ago

Hi, @goldsam!

That would be definitely appreciated.

This is already part of the plan since I didn't plan to put the burden of providing the Yrs binaries on the clients anyway, but this would only be relevant after finishing the bindings.

goldsam commented 1 year ago

It seems like your making amazing progress very quickly. I'll see what I can do

goldsam commented 1 year ago

@LSViana I've made a little bit of progress in generating and packaging the binaries. The more challenging part of this task is selection of the binary to load at runtime. I was going to take some inspiration from SkiaSharp and Tesseract (the later of which uses Interop.net internally) but wanted to first get your opinion before doing any work you might disagree with.

goldsam commented 1 year ago

@LSViana Following your feedback from #19 , my updated build against main of LSViana/y-crdt shows 4 tests failing (165 passing). Is that expected?

  Failed ObserveDeepHasPathWhenAdded [43 ms]
  Error Message:
     Expected: 1
  But was:  3

  Stack Trace:
     at YDotNet.Tests.Unit.Arrays.ObserveDeepTests.ObserveDeepHasPathWhenAdded() in C:\Users\samgo\Documents\projects\ydotnet\Tests\YDotNet.Tests.Unit\Arrays\ObserveDeepTests.cs:line 61

  Failed ObserveDeepHasPathWhenRemoved [1 ms]
  Error Message:
     Expected: 1
  But was:  2

  Stack Trace:
     at YDotNet.Tests.Unit.Arrays.ObserveDeepTests.ObserveDeepHasPathWhenRemoved() in C:\Users\samgo\Documents\projects\ydotnet\Tests\YDotNet.Tests.Unit\Arrays\ObserveDeepTests.cs:line 128

  Skipped Dispose [< 1 ms]
  Skipped GetNewKeyReturnsNull [< 1 ms]
  Skipped InsertDifferentTypeOnExistingKey [< 1 ms]
  Skipped InsertEqualTypeOnExistingKey [< 1 ms]
  Skipped InsertSameInstanceOnMultipleKeys [< 1 ms]
  Failed LengthIsCorrectForAlphanumericAndEmojiCharacters [< 1 ms]
  Error Message:
     Expected: 10
  But was:  8

  Stack Trace:
     at YDotNet.Tests.Unit.Texts.LengthTests.LengthIsCorrectForAlphanumericAndEmojiCharacters() in C:\Users\samgo\Documents\projects\ydotnet\Tests\YDotNet.Tests.Unit\Texts\LengthTests.cs:line 52

  Skipped ObserveDeepHasPathWhenAdded [< 1 ms]
  Failed ReturnsTheFullText [1 ms]
  Error Message:
     String lengths are both 7. Strings differ at index 6.
  Expected: "Star. ?"
  But was:  "Star. ?"
  -----------------^

  Stack Trace:
     at YDotNet.Tests.Unit.Texts.StringTests.ReturnsTheFullText() in C:\Users\samgo\Documents\projects\ydotnet\Tests\YDotNet.Tests.Unit\Texts\StringTests.cs:line 21

  Skipped CreateAndApplySnapshot [< 1 ms]
  Skipped CreateAndApplySnapshot [< 1 ms]
LSViana commented 1 year ago

@LSViana I've made a little bit of progress in generating and packaging the binaries. (...)

That looks great to me! I see you were able to build for multiple platforms successfully and that's nice.

I think we can work on downloading the correct binaries for the running platform & architecture during the build step from the published artifacts, right? If the file is already downloaded, we just skip not to slow down every build.

I don't have lots of experience with that, to be honest, but what you started there is promising and if I can help with it after I'm done with the library bindings, I'll definitely try. 🙂

@LSViana Following your feedback from https://github.com/LSViana/ydotnet/issues/19 , my updated build against main of LSViana/y-crdt shows 4 tests failing (165 passing). Is that expected?

I just noticed that some tests are failing on Windows and, no, it's not expected. I've been mixing my development time between Unix and Windows machines to ensure the bindings are working on both sides but I let these issues go unnoticed.

The issues with the ObserveDeep* tests have probably already been fixed by this commit on my fork of Yrs. Please, can you check if you have that update locally?

The issues with emojis are probably due to the usage of UTF-16 characters because the Doc constructor uses UTF-8 characters by default. But it does support UTF-16 characters and I missed adding it to this test, I'll update this soon.

Additionally, I talked to the maintainers of y-crdt and they mentioned that it probably makes sense to update the C# code to use UTF-16 by default when creating Doc instances because that's already the language default. I'll also push updates for this soon.

LSViana commented 1 year ago

@goldsam An update here: the issues with emoji and other special characters should be fixed by now after the commits I pushed today. I tested them both on Windows and macOS and the tests are all passing now.

Beyond fixing these 2 failing tests, I actually applied a library-wide update to read and write string instances transported through the FFI to make sure we have full control instead of relying on the default mechanisms that can't handle emojis and other special characters.

If you have any issues with these latest updates, please let me know.

SebastianStehle commented 11 months ago

@goldsam are you still working on this? I would really like to use yjs in a .net project so I am pushing this and the server components. But I have almost zero experience with rust.

goldsam commented 11 months ago

@SebastianStehle Did you not see the link to my repo above?

Skyppid commented 10 months ago

Same here, been looking for Yjs compatible port for C# that supports the current Yjs featureset. I also never worked with Rust and it's really a turn off having to struggle with that. A pipeline for this repository that builds NuGet packages with the necessary libaries would be amazing :)

The pipeline looks good, but are there any packages? Or what's the status of it @goldsam?

LSViana commented 10 months ago

Hi, @Skyppid!

There's a PR that is supposed to fix that and should be merged really soon (see #55).

Skyppid commented 10 months ago

Ah I saw this one, but didn't realize that it also included the pipeline. Quite a huge PR. Nonetheless I'm looking forward to see it merged! Definetly gonna try it out and help in case I find any issues. Gonna need this for a project I've been wanting to do for a long time now and it's the only missing puzzle piece 😄

Thanks for the quick update :)

LSViana commented 10 months ago

Hi, @Skyppid!

PR #55 was just merged and you should be able to see some improvements to the strategy to include binaries for each platform in there now.

If you have questions, please let me know.

Skyppid commented 10 months ago

Amazing, thank you guys! I'll be checking it out as soon as I got time. I'll support wherever I can :) Do I see it correctly that Ydotnet does not only provide the basic Y components but also some management classes already for .NET backends?

I'll use it with SignalR, maybe I can provide a basic framework for people to use it without much work on their own if @SebastianStehle isn't doing this already.

SebastianStehle commented 10 months ago

Hi, yes YDotnet provides a websocket backend now. But there is no clustering feature yet. I was playing around here: https://github.com/SebastianStehle/ydotnet/tree/main/YDotNet.Server/Clustering, but I never put it in the pr.

The reason is that I don't have the opportunity to battle test it at the moment. The question where to handle persistency and so on is challenging because the server is stateful. In general I think the best solution would be to use something like actors or grains where exactly on server stores a single document (e.g. with Microsoft Orleans), but this has other challenges around cluster management.

So for my use case, where I have only very few people working with the yjs functionality I have decided to declare one node as yjs node. So I basically have N normal nodes and one yjs node (with a environment variable) and the UI connects to this dedicated node.

Skyppid commented 10 months ago

Alright, fair points. Well for me it's not necessary as of now. I'm only building a prototype for now so no need for clustering yet. Is the websocket implementation compatible with Yjs' websockets (looks like it from what I saw)?

SebastianStehle commented 10 months ago

Yes, it is. It uses the same encoders and stuff from y-protocols, e.g. https://github.com/LSViana/ydotnet/blob/main/YDotNet.Server.WebSockets/EncoderExtensions.cs

And in the demo I use the normal websocket provider: https://github.com/LSViana/ydotnet/blob/main/Demo/Client/src/context/yjsContext.tsx

It does not implement auth yet, but the y-websocket provider also does not support it.

Skyppid commented 10 months ago

Well no auth is an issue, but also something I can ignore for now. Shouldn't be too hard to implement, though. Something like that: https://github.com/yjs/y-websocket/issues/7#issuecomment-623114183

SebastianStehle commented 10 months ago

No auth is not entirely correct. You can implement the normal auth, but I think some headers are not supported for web sockets and stuff like this, therefore it can make sense to implement in the websocket part directly and not outside.