Upstream FluffyBox patches

[TheOneWithTheBraid]

Somehow reached at not making this a breaking change...

• exposes BoxCollection
• prepares to add additional web backends

Fixes: #939

[themisir]

Thank you for the contribution! Not gonna lie there's lots of changes and it scares me a bit 😅 So I'm gonna ask a few questions before reviewing it.

• Can you please write small overview of what's done in this PR. Like what's added which parts are changed. Just a few lines of overview.
• What's purpose of BoxCollection? (please note that it's not a concern, just a question since I haven't fully checked the code, I just wanna know what's its purpose, how it differs from existing Box interface, etc..)
• Does it requires any steps before building (i guess it does because web tests are failing)? For example afaik your previous PR states that some scripts needs to be built manually for web. If possible can you write a small step-by-step guide as a reply to this thread so I can state this in hive documentation.

Again sorry for requesting additional stuff, but the proposed changes are a bit bigger than what we usually get, so I want to make sure it won't break anything unintentionally.

[TheOneWithTheBraid]

Thanks for the review.

• let's answer the second question first: As of now, Hive creates an indexed DB database for each box - containing just one objectstore. We benchmarked that this architecture tremendously slows down huge read- and write operations. That's why FluffyBox was created as an abstraction layer adding support for different objectstores (CollectionBox) to Hive by introducing a BoxCollection (representing an indexed DB database).
• files changed:
  • the backend stuff: changes allowing to pass a collection to aside of the box name in order to allow corresponding well... collections
  • web stuff:
    • preparation for support of different web backend backends (related to feat: allow indexed DB in web worker #933 but not yet ready)
    • changes in indexed DB interface in order to use the collection as objectstore if provided
  • the rest seems obvious to me...
• special notices for builds: not yet; will follow together with the final fix for feat: allow indexed DB in web worker #933

[TheOneWithTheBraid]

In regard of the failing tests, honestly no idea, locally, they pass for me. Maybe we can investigate this together?

Test outputs

 dart test
Building package executable... (7.2s)
Built test:test.
00:03 +0: test/integration/ignore_type_id_test.dart: ignore typeId with IgnoredTypeAdapter
Registering type adapters for dynamic type is must be avoided, otherwise all the write requests to Hive will be handled by given adapter. Please explicitly provide adapter type on registerAdapter method to avoid this kind of issues. For example if you want to register MyTypeAdapter for MyType class you can call like this: registerAdapter<MyType>(MyTypeAdapter())
00:21 +25: test/tests/registry/type_registry_impl_test.dart: TypeRegistryImpl .registerAdapter() dynamic type
Registering type adapters for dynamic type is must be avoided, otherwise all the write requests to Hive will be handled by given adapter. Please explicitly provide adapter type on registerAdapter method to avoid this kind of issues. For example if you want to register MyTypeAdapter for MyType class you can call like this: registerAdapter<MyType>(MyTypeAdapter())
00:21 +35: test/tests/registry/type_registry_impl_test.dart: TypeRegistryImpl .ignoreTypeId() registers IgnoredTypeAdapter
Registering type adapters for dynamic type is must be avoided, otherwise all the write requests to Hive will be handled by given adapter. Please explicitly provide adapter type on registerAdapter method to avoid this kind of issues. For example if you want to register MyTypeAdapter for MyType class you can call like this: registerAdapter<MyType>(MyTypeAdapter())
00:21 +36: test/tests/registry/type_registry_impl_test.dart: TypeRegistryImpl .ignoreTypeId() duplicte typeId
Registering type adapters for dynamic type is must be avoided, otherwise all the write requests to Hive will be handled by given adapter. Please explicitly provide adapter type on registerAdapter method to avoid this kind of issues. For example if you want to register MyTypeAdapter for MyType class you can call like this: registerAdapter<MyType>(MyTypeAdapter())
00:25 +164: test/tests/backend/vm/storage_backend_vm_test.dart: StorageBackendVm .initialize() (not lazy) recoveryOffset with crash recovery
Recovering corrupted box.
00:25 +167: test/tests/backend/vm/storage_backend_vm_test.dart: StorageBackendVm .initialize() (lazy) recoveryOffset with crash recovery
Recovering corrupted box.
00:25 +184: test/tests/backend/vm/backend_manager_test.dart: BackendManager findHiveFileAndCleanUp only compact file
Restoring compacted file.
00:30 +392: All tests passed! 

[themisir]

In regard of the failing tests, honestly no idea, locally, they pass for me. Maybe we can investigate this together?

Test outputs

No problem, don't worry I'll take a look into it. I thought maybe it's because of that required manual build step.

[krille-chan]

Hi there :) I'm just curious if there are any news on this, as it would mean that I can finally ditch FluffyBox

[TheOneWithTheBraid]

At the moment, the test behavior is somehow unexpected: Running the JS-specific tests (e.g. on BackendManager) perfectly works without any error reports, anyway, when running all tests, the majority is failing.

dart test -pfirefox test/tests/backend/js/storage_backend_js_test.dart # works
dart test -pfirefox # won't work

[themisir]

At the moment, the test behavior is somehow unexpected: Running the JS-specific tests (e.g. on BackendManager) perfectly works without any error reports, anyway, when running all tests, the majority is failing.

dart test -pfirefox test/tests/backend/js/storage_backend_js_test.dart # works
dart test -pfirefox # won't work

That's very strange.. Let's merge it, and then see if we can find what causes tests to break, maybe it's some sort of dart test runner related issue.

[TheOneWithTheBraid]

I think it's related to the way the tests access the HiveImpl... It would actually be much cleaner to access the interface via Hive instead of directly instancing HiveImpl each time again.

[TheOneWithTheBraid]

Could these changes be published to pub.dev?

[themisir]

Could these changes be published to pub.dev?

Done!

[baumths]

I think we should've updated the docs before publishing 😕

[themisir]

I think we should've updated the docs before publishing 😕

Afaik this patch doesn't changes any exists APIs so existing docs should be fine for new version, and we can document new features.