search

Rudiksz / couchbase_lite_dart

Dart implementation of the Couchbase Lite, an embedded, NoSQL JSON Document Style database.
BSD 3-Clause "New" or "Revised" License
24 stars 3 forks source link

Null Safe Support #13

Open richard457 opened 3 years ago

richard457 commented 3 years ago
  1. Flutter v2 is out with sound null safety
  2. It would be nice to have this in the package.
Rudiksz commented 3 years ago

It was in the works, but with the new "stable" release of Dart they introduced some breaking changes in dart:ffi, Changes that somehow I had no idea are coming even though I develop and test on the master branch.

It's in the works.

richard457 commented 3 years ago

@Rudiksz looked at the codebase, but I think in order for me to contribute, it's good if we can have a talk so we can have some common understanding.

Rudiksz commented 3 years ago

I have almost completed the migration and I hope I'll be able to publish the new version this week.

richard457 commented 3 years ago

@Rudiksz I really like this package and I am doing extensive experiments with it, will be happy to write documentation and some writing about it and also to contribute moving forward. Looking to #null safety

Rudiksz commented 3 years ago

You can find a prerelease on the ffi1.0 branch: https://github.com/Rudiksz/couchbase_lite_dart/tree/ffi1.0

Pub release: https://pub.dev/packages/couchbase_lite_dart/versions/0.5.0-nullsafety.2

A few things to note:

No API changes were made so it should work with existing code, as long as the code is null-safe. Added empty constructors the Database, Document, Query and Replicator classes to create 'empty' classes - this needs some extra testing.

richard457 commented 3 years ago

@Rudiksz Thanks for the good work, doing testing with the existing app now.

richard457 commented 3 years ago

Tested the new API here is my finding

  1. The API did not change ( :) happy)
  2. When building a windows app the build does not copy the dynlib inbuilt app release so I have to manually copy the folder into the release folder
  3. The android does not work (No binaries known)
Rudiksz commented 3 years ago

1) The API did not change ( :) happy)

Yes. but with a big caveat. While it's nullsafe for Dart, it is definitely not null-safe yet, and there's a few extra internal checks I need to implement to make the C memory management truly nullsafe. At the moment you should consider every class (Database, Document, Query, Replicator) as nullable and use their isEmpty method before performing operations on them - unless you know for sure you initialised them. isEmpty basically checks that the underlying C pointer is not a nullpointer. There are still some cases that would cause a hard crash on the C side. It's definitely not production ready yet.

I'm still deciding how to handle these checks on the Dart side. Take the following Query class for example.

final query = Query.empty();
final string = query.explain();

This will crash the app with a nullptr exception. Of course I will add a guard in the method, but the question is: should it throw a Dart excpetion, or just fail silently and return an empty string? I'm leaning to the second option, but I'm open to other opinions.

2) When building a windows app the build does not copy the dynlib inbuilt app release so I have to manually copy the folder into the release folder

This was the case also before. When Flutter builds a windws app it only copies the flutter dll at the moment. Maybe there's a gradle setting or something to include custom artifacts in the release folder, but I haven't looked yet.

Flutter's build process does not seem to support bundling dynamic libraries from external packages. All the examples they have is how to link them if you are building an app. Linking in a package and then requiring the package does not automatically link them to you app. That's why you have to manually copy the libraries in your app's folder when you are developing.

What is new I guess, is that the windows dll is no longer bundled with the package, and you need to manually copy it to your app. Just like the android and macos files. I felt there's no reason to keep it separate and it makes the pub package itself much smaller and releases that don't affect the C library (I do have some custom hacks) much leaner to upgrade to.

3) The android does not work (No binaries known)

I uploaded them to the release page. Did some initial testing only for now.

richard457 commented 3 years ago

did another deep testing now on the release app, the app works just fine on the same machine where the development is, but then run the same app to another machine it does not work, even when copied the folder "dynlib" in the same folder. I can confirm that the issue is how couchbase_lite_dart is loading .dll files. @Rudiksz

richard457 commented 3 years ago
  1. The API did not change ( :) happy)

Yes. but with a big caveat. While it's nullsafe for Dart, it is definitely not null-safe yet, and there's a few extra internal checks I need to implement to make the C memory management truly nullsafe. At the moment you should consider every class (Database, Document, Query, Replicator) as nullable and use their isEmpty method before performing operations on them - unless you know for sure you initialised them. isEmpty basically checks that the underlying C pointer is not a nullpointer. There are still some cases that would cause a hard crash on the C side. It's definitely not production ready yet.

I'm still deciding how to handle these checks on the Dart side. Take the following Query class for example.

final query = Query.empty();
final string = query.explain();

This will crash the app with a nullptr exception. Of course I will add a guard in the method, but the question is: should it throw a Dart excpetion, or just fail silently and return an empty string? I'm leaning to the second option, but I'm open to other opinions.

  1. When building a windows app the build does not copy the dynlib inbuilt app release so I have to manually copy the folder into the release folder

This was the case also before. When Flutter builds a windws app it only copies the flutter dll at the moment. Maybe there's a gradle setting or something to include custom artifacts in the release folder, but I haven't looked yet.

Flutter's build process does not seem to support bundling dynamic libraries from external packages. All the examples they have is how to link them if you are building an app. Linking in a package and then requiring the package does not automatically link them to you app. That's why you have to manually copy the libraries in your app's folder when you are developing.

What is new I guess, is that the windows dll is no longer bundled with the package, and you need to manually copy it to your app. Just like the android and macos files. I felt there's no reason to keep it separate and it makes the pub package itself much smaller and releases that don't affect the C library (I do have some custom hacks) much leaner to upgrade to.

  1. The android does not work (No binaries known)

I uploaded them to the release page. Did some initial testing only for now.

This will crash the app with a nullptr exception. Of course I will add a guard in the method, but the question is: should it throw a Dart excpetion, or just fail silently and return an empty string? I'm leaning to the second option, but I'm open to other opinions. I think for this case failing silently is not bad I like non-blocking error and throwing dart error I guess will break the entire execution

richard457 commented 3 years ago

bundling dynamic libraries from external packages

Was looking to this https://pub.dev/packages/external_asset_bundle as a way we can load external library but would like to see other options, still Researching.

richard457 commented 3 years ago

bundling dynamic libraries from external packages

https://github.com/flutter/flutter/issues/81978

Rudiksz commented 3 years ago

Copying the dll file should work, but probably I have to fix in the library, because I changed that part.

richard457 commented 3 years ago

@Rudiksz any update?

Rudiksz commented 3 years ago

Try the new 0.5.0-nullsafety.3 version.

Please note that I changed the location of the libraries (again), from dynlib to vendor/cblite. This is relative to your project root. I'm not aware of any accepted standard in Dart and Flutter to where to put third party packages, and vendor being the standard in some other languages I like it more than dynlib. I will keep support for vendor even if a different location emerges in the dart community.

Edit: for windows, when distributing exe files, the dll can be either next to the exe file (in the same folder), or in the vendor/cblite subfolder relative to the exe.

richard457 commented 3 years ago

should the .dll files from v.0.5.0-null safety.2 be used? or there are other compiled files for this 0.5.0-nullsafety.3 versions.?

richard457 commented 3 years ago

I agree with you, the vendor seems familiar with the developer community

Rudiksz commented 3 years ago

should the .dll files from v.0.5.0-null safety.2 be used? or there are other compiled files for this 0.5.0-nullsafety.3 versions.?

The same same ones are good. There's no other change in this release.