Migrate hive to dart nnbd #521

KalilDev · 2020-12-19T22:34:17Z

For now, package:hive was migrated, and every test passes, but there are some hacks on the test's codegen and package:crypto and package:mockito have been overriden with git versions which were not published on pub.dev. The tests also depend on the legacy package:dartx and package:pointycastle, so they can only be ran with --no-sound-null-safety.

The release is blocked until at least package:crypto has an published non-nullable version.

Ill start working on the hive_generator, as hive needed to be migrated first because it is an direct dependency of the packages which would use the generator.

* Using `dart migrate`

* Will be needed for mock codegen

* Also add missing imports and remove duplicated methods and accessors * The codegen was made with an modified build of dart-lang/mockito@c387e34 that adds an `as $Type` after each noSuchMethod invocation to fix the static type error of returning dynamic instead of the correct type

* Because every element is of type E, and E extends HiveObject, the type E can never be nullable, and therefore no element can be null, otherwise an static type error would happen, so the check would always pass

* MockHiveObject does not need to exist, i shouldve used an TestHiveObject which extends HiveObject. * So, create the class, use it in the tests and remove HiveObject from the GenerateMocks annotation.

* Remove the MockHiveObject class

* The null check operator needed to be inside an ${} block on the concatenation, so because it wasn't, it was interpreted as an '!' String. * I did not use it now because both Object and Object? have the toString method, and althogh an invalid path would be generated in case path was null, this will never happen, because of the ArgumentError.nonNull on the start of the functions.

* hashCode, == and toString are used internally in Maps, Sets, and other places of the dart standard libray, and returning null is not allowed, therefore, the functionality gets broken. * As we do not mock any of these, using the standard Mock implementation is fine, as it does not return null. * This is NOT ideal, something should be done once the non-nullable Mockito api and codegen get stablized.

* It was accidentally removed on the migration

* registry is an non-nullable late property, and it is set on StorageBackendVM.initialize, but we do not call it on the read/write tests, therefore they throw, so to fix it, we manually set the registry before the read/write tests.

KalilDev · 2021-01-24T16:50:56Z

Tests for the codegen are looking good. Didn't find an problem reviewing it manually, and dart analyze does not complain either.

* BinaryWriter contains the correct nullability, but BinaryWriterImpl was wrong

KalilDev · 2021-01-24T18:47:36Z

@themisir soo, only hive_flutter remains to be migrated, and the current version for it is 0.3.1, and according to dart.dev, the new version can either be 0.4.0-nullsafety.0 or 1.0.0-nullsafety.0. which one will we go with? The same for hive_generator, which is 0.8.2 and can be either 0.9.0-nullsafety.0 or 1.0.0-nullsafety.0.

And, should we finish reviewing and publishing the core packages (hive and hive_generator) before migrating hive_flutter, or should i migrate it in this PR?

Im waiting for further instructions so we can move hive forward!

themisir · 2021-01-24T20:54:52Z

@themisir soo, only hive_flutter remains to be migrated, and the current version for it is 0.3.1, and according to dart.dev, the new version can either be 0.4.0-nullsafety.0 or 1.0.0-nullsafety.0. which one will we go with? The same for hive_generator, which is 0.8.2 and can be either 0.9.0-nullsafety.0 or 1.0.0-nullsafety.0.

And, should we finish reviewing and publishing the core packages (hive and hive_generator) before migrating hive_flutter, or should i migrate it in this PR?

Im waiting for further instructions so we can move hive forward!

I think increasing major version (1.X.Y+nullsafety.Z) will work for us cause the new library will not be compatible with previous version because it requires explicitly enabling NNBD. Correct me if I'm wrong.

And about hive_flutter we can either work on it later or publish them all at once. It doesn't matter I think.

Also I have a question. I haven't worked on null-safety enabled projects before so, will the NNBD version work with the previous versions of the flutter or dart? I mean at least currently stable versions (which comes with NNBD disabled). Or we'll have to maintain nndb and non-nndb codebase separately?

(I've inspected some of flutter's own source code on my localhost, currently my flutter environment set to not use NNBD but flutter source code contains NNDB code.) If you have any idea about that please let me know. 🙏

felangel · 2021-01-25T04:10:53Z

the new version can either be 0.4.0-nullsafety.0 or 1.0.0-nullsafety.0. which one will we go with? The same for hive_generator, which is 0.8.2 and can be either 0.9.0-nullsafety.0 or 1.0.0-nullsafety.0.

I would recommend 0.4.0-nullsafety.0 and 0.9.0-nullsafety.0 because dart semver will treat those as major version bumps (since the version is less than 1.0.0) so a 1.0.0 release is not necessary.

And about hive_flutter we can either work on it later or publish them all at once. It doesn't matter I think.

It would be great if nullsafety releases could be published as soon as possible for hive and hive_generator so that other packages which depend on them can start migrating right away.

felangel · 2021-01-26T23:44:57Z

After testing these changes I think I've found a bug in Hive.close().

I would expect the following to be valid:

Hive.init(path);
await Hive.close();

Hive.init(path); // this throws HiveError: Instance has already been initialized.
await Hive.close();

This could be solved by resetting the homePath to null when close is called at https://github.com/hivedb/hive/pull/521/files#diff-5405afd0b66478f672f1f6a400d2400403aac2b2993d57fb313a64aaa902cd47L207.

  @override
  Future<void> close() {
    var closeFutures = _boxes.values.map((box) {
      return box.close();
    });

    homePath = null;
    return Future.wait(closeFutures);
  }

Another option would be to revert 59ad540#r46404472 as it introduces a breaking change and no longer allows for multiple Hive instances to exist simultaneously.

Let me know what you think 👍

themisir · 2021-01-27T17:38:13Z

After testing these changes I think I've found a bug in Hive.close().

I would expect the following to be valid:
Hive.init(path);
await Hive.close();

Hive.init(path); // this throws HiveError: Instance has already been initialized.
await Hive.close();
This could be solved by resetting the homePath to null when close is called at https://github.com/hivedb/hive/pull/521/files#diff-5405afd0b66478f672f1f6a400d2400403aac2b2993d57fb313a64aaa902cd47L207.
  @override
  Future<void> close() {
    var closeFutures = _boxes.values.map((box) {
      return box.close();
    });

    homePath = null;
    return Future.wait(closeFutures);
  }
Another option would be to revert 59ad540#r46404472 as it introduces a breaking change and no longer allows for multiple Hive instances to exist simultaneously.

Let me know what you think 👍

Initializing hive multiple times causes weird errors. So Instead of initializing it multiple times you can construct HiveImpl instance and use it.

var hive1 = HiveImpl();
hive1.init(path);
await hive1.close();

var hive2 = HiveImpl();
hive2.init(path);
await hive2.close();

felangel · 2021-01-29T05:05:55Z

@themisir thanks for the clarification!

Is there anything blocking a release of hive and hive_generator? It's currently blocking felangel/bloc#2117. Let me know if I can help in any way 👍

KalilDev · 2021-01-30T03:50:36Z

the new version can either be 0.4.0-nullsafety.0 or 1.0.0-nullsafety.0. which one will we go with? The same for hive_generator, which is 0.8.2 and can be either 0.9.0-nullsafety.0 or 1.0.0-nullsafety.0.

I would recommend 0.4.0-nullsafety.0 and 0.9.0-nullsafety.0 because dart semver will treat those as major version bumps (since the version is less than 1.0.0) so a 1.0.0 release is not necessary.

And about hive_flutter we can either work on it later or publish them all at once. It doesn't matter I think.

It would be great if nullsafety releases could be published as soon as possible for hive and hive_generator so that other packages which depend on them can start migrating right away.

Alright, thanks for the tip! I will promptly update the versions and write a changelog for the packages.

@themisir thanks for the clarification!

Is there anything blocking a release of hive and hive_generator? It's currently blocking felangel/bloc#2117. Let me know if I can help in any way 👍

I think there isn't, the tests are passing, including the integration tests, and the hive_generator code looks good, but i haven't tested the new hive manually in an real enviroment. If you could, that would be of great help, ensuring that it can be merged safely, otherwise i can do it after the deadline im in.

I'm sorry for the late response, these days i've been busy with an deadline for 02/01, so i haven't had the time to follow up this pr.

felangel · 2021-01-30T03:52:57Z

@themisir no problem I completely understand! I’ve upgraded hydrated_bloc to use this version and everything seems to be working 👍

themisir · 2021-01-30T08:01:21Z

@themisir thanks for the clarification!

Is there anything blocking a release of hive and hive_generator? It's currently blocking felangel/bloc#2117. Let me know if I can help in any way 👍

I'll work on this pr today. And publish if everything goes okay.

* also write the tiny changelog

* The generator does not depend on changes made on package:hive

* Preparing for publishing

* prevents the list from having to grow possibly many times

* These primitives must not be null. To write an null value, use write.

themisir · 2021-01-30T16:39:33Z

I've just completed testing hive and hive_generator.

@KalilDev can you upgrade minor version instead of major as @felangel said? I'll merge this PR after that.

I would recommend 0.4.0-nullsafety.0 and 0.9.0-nullsafety.0 because dart semver will treat those as major version bumps (since the version is less than 1.0.0) so a 1.0.0 release is not necessary.

KalilDev · 2021-01-30T16:42:46Z

I've just completed testing hive and hive_generator.

@KalilDev can you upgrade minor version instead of major as @felangel said? I'll merge this PR after that.

I would recommend 0.4.0-nullsafety.0 and 0.9.0-nullsafety.0 because dart semver will treat those as major version bumps (since the version is less than 1.0.0) so a 1.0.0 release is not necessary.

I just pushed hive_generator to 0.9.0-nullsafety.0, here, is there anything else left?

themisir · 2021-01-30T16:55:17Z

@KalilDev

Can you also set hive version to 1.6.0-nullsafety.0?

themisir · 2021-01-30T17:16:04Z

Nevermind I'll do it myself after merging. Thanks for the contribution 🙏

KalilDev · 2021-01-30T17:20:30Z

@KalilDev

Can you also set hive version to 1.6.0-nullsafety.0?

Oh, sorry, i had to leave home for a bit.

Nevermind I'll do it myself after merging. Thanks for the contribution pray

Alright!! No problem, man! Thanks for merging!

themisir · 2021-01-30T17:24:42Z

Oh, sorry, i had to leave home for a bit.

No problem ✌

themisir · 2021-01-30T17:45:47Z

@KalilDev may I know how did you setup your environment (flutter cli, vscode, etc...) to use null safety? I couldn't setup mine unfortunately :(

KalilDev · 2021-01-30T18:09:06Z

@KalilDev may I know how did you setup your environment (flutter cli, vscode, etc...) to use null safety? I couldn't setup mine unfortunately :(

For it to work dart --version needs to be greater or equal to 2.12.0. For that i'm using the dart sdk from flutter on the master channel, but it works with the beta channel too. The dart executable that flutter ships is in $yourFlutterRoot/bin/cache/dart-sdk/bin/, so that folder needs to be in $PATH

I'm using vscode as the ide, and it didn't need any special configuration, and for using nnbd hive on a package, the dart sdk constraint needs to be >= 2.12.0-0 and the hive dependency needs to be overriden with the following pubspec entries:

dependencies:
  hive: ^1.6.0-nullsafety.0

dev_dependencies:
  hive_generator: ^0.9.0-nullsafety.0

dependency_overrides:
  hive:
    git:
      url: [email protected]:hivedb/hive.git
      ref: nnbd
      path: hive
  hive_generator:
    git:
      url: [email protected]:hivedb/hive.git
      ref: nnbd
      path: hive_generator

KalilDev added 30 commits December 14, 2020 20:28

hive: upgrade packages

ccbc576

hive: Override package:crypto for null-safety

f8b10f6

hive: Add migration hints

f86f55b

hive_generator: Use hive from path

810ac3d

hive: Migrate to null-safe dart

6b73cfb

* Using `dart migrate`

hive: Manually finish the migration

73c1d0d

hive: turn some props nullable &finish migration

c0ba538

hive: finish manually migrating tests

522a0bc

hive: Fix some lints

90e0638

hive: override mockito and pin crypto

0cfd503

hive: backend_manager: Fix path nullability

8f4672a

hive: type_registry: Expose ResolvedAdapter

c51172b

* Will be needed for mock codegen

hive: test: Refactor to use @GenerateMocks

242bdba

hive: test: Move back mocks to a single file

890af71

hive: test: Do not make test groups async

c88a4dc

hive: tests: Do not throw on missing stubs

044a252

hive: test: Fix null assertions that always fail

7a52f29

hive: Turn some props rw for testing

4848cae

hive: list_impl: Remove element check

380f2e1

* Because every element is of type E, and E extends HiveObject, the type E can never be nullable, and therefore no element can be null, otherwise an static type error would happen, so the check would always pass

hive: keystore: fix sloppy cast from migration

9db01c8

hive: test: Fix TestHiveObject derp

0b33df4

* MockHiveObject does not need to exist, i shouldve used an TestHiveObject which extends HiveObject. * So, create the class, use it in the tests and remove HiveObject from the GenerateMocks annotation.

hive: test: Run codegen

8287860

* Remove the MockHiveObject class

hive: frame_io_helper_test: unbork from migration

65d0c43

hive: keystore: Fix cast from migration

07d1761

hive: hive_list_impl: Add back box check

b8fb36b

* It was accidentally removed on the migration

hive: test: Fix null assertions that always threw

733b24a

hive: test: storage_backend_vm: Init registry

5282621

* registry is an non-nullable late property, and it is set on StorageBackendVM.initialize, but we do not call it on the read/write tests, therefore they throw, so to fix it, we manually set the registry before the read/write tests.

hive: binary_writer: Fix write param nullability

cb737fe

* BinaryWriter contains the correct nullability, but BinaryWriterImpl was wrong

themisir changed the base branch from master to nnbd January 24, 2021 20:57

felangel mentioned this pull request Jan 29, 2021

feat(hydrated_bloc)!: null safety felangel/bloc#2117

Merged

7 tasks

KalilDev added 6 commits January 30, 2021 12:02

hive_generator: Change version to 0.9.0-nullsafety.0

a898cae

* also write the tiny changelog

hive_generator: Lower hive version constraint

282998e

* The generator does not depend on changes made on package:hive

hive_generator: Do not override hive from path

bc30bff

* Preparing for publishing

hive: Changelog for 2.0.0-nullsafety.0

7f5bdc3

binary_reader: use List.filled on readHiveList

806c6ac

* prevents the list from having to grow possibly many times

hive: binary_writer: checkNotNull before write

6772fbd

* These primitives must not be null. To write an null value, use write.

themisir merged commit 3873fae into isar:nnbd Jan 30, 2021

themisir changed the title ~~[WIP] DO NOT MERGE: Migrate hive to dart nnbd~~ Migrate hive to dart nnbd Jan 30, 2021

themisir linked an issue Jan 30, 2021 that may be closed by this pull request

Null-safety #496

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate hive to dart nnbd #521

Migrate hive to dart nnbd #521

KalilDev commented Dec 19, 2020

KalilDev commented Jan 24, 2021

KalilDev commented Jan 24, 2021

themisir commented Jan 24, 2021

felangel commented Jan 25, 2021 •

edited

Loading

felangel commented Jan 26, 2021 •

edited

Loading

themisir commented Jan 27, 2021

felangel commented Jan 29, 2021 •

edited

Loading

KalilDev commented Jan 30, 2021

felangel commented Jan 30, 2021

themisir commented Jan 30, 2021 •

edited

Loading

themisir commented Jan 30, 2021 •

edited

Loading

KalilDev commented Jan 30, 2021

themisir commented Jan 30, 2021

themisir commented Jan 30, 2021

KalilDev commented Jan 30, 2021

themisir commented Jan 30, 2021

themisir commented Jan 30, 2021 •

edited

Loading

KalilDev commented Jan 30, 2021 •

edited

Loading

Migrate hive to dart nnbd #521

Migrate hive to dart nnbd #521

Conversation

KalilDev commented Dec 19, 2020

KalilDev commented Jan 24, 2021

KalilDev commented Jan 24, 2021

themisir commented Jan 24, 2021

felangel commented Jan 25, 2021 • edited Loading

felangel commented Jan 26, 2021 • edited Loading

themisir commented Jan 27, 2021

felangel commented Jan 29, 2021 • edited Loading

KalilDev commented Jan 30, 2021

felangel commented Jan 30, 2021

themisir commented Jan 30, 2021 • edited Loading

themisir commented Jan 30, 2021 • edited Loading

KalilDev commented Jan 30, 2021

themisir commented Jan 30, 2021

themisir commented Jan 30, 2021

KalilDev commented Jan 30, 2021

themisir commented Jan 30, 2021

themisir commented Jan 30, 2021 • edited Loading

KalilDev commented Jan 30, 2021 • edited Loading

felangel commented Jan 25, 2021 •

edited

Loading

felangel commented Jan 26, 2021 •

edited

Loading

felangel commented Jan 29, 2021 •

edited

Loading

themisir commented Jan 30, 2021 •

edited

Loading

themisir commented Jan 30, 2021 •

edited

Loading

themisir commented Jan 30, 2021 •

edited

Loading

KalilDev commented Jan 30, 2021 •

edited

Loading