Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Type registration improvements idea #163

Closed
SergeShkurko opened this issue Dec 29, 2019 · 10 comments
Closed

Type registration improvements idea #163

SergeShkurko opened this issue Dec 29, 2019 · 10 comments
Labels
enhancement New feature or request

Comments

@SergeShkurko
Copy link
Contributor

SergeShkurko commented Dec 29, 2019

Simplify custom type registration
partially hide typeid for registration

The bottom line is that the id is initialized only once and cannot be changed in the future. it would be more logical to combine it with the model itself

now:

// model
@HiveType()
class User extends HiveObject {
  User({
    @required this.id,
    @required this.email,
  });

  static const int typeId = 1;

  int id;
  String email;
}

// adapter

class UserAdapter extends TypeAdapter<User> {
  @override
  User read(BinaryReader reader) {
    var numOfFields = reader.readByte();
    var fields = <int, dynamic>{
      for (var i = 0; i < numOfFields; i++) reader.readByte(): reader.read(),
    };
    return User();
  }

  @override
  void write(BinaryWriter writer, User obj) {
    writer..writeByte(0);
  }
}

// registration
Hive.registerAdapter(UserAdapter(), User.typeId)

After:

// model
@HiveType(typeId: 1)
class User extends HiveObject {
  User({
    @required this.id,
    @required this.email,
  });

  int id;
  String email;
}

// adapter

class UserAdapter extends TypeAdapter<User> {
  @override
  int get typeId => 1;

  @override
  User read(BinaryReader reader) {
    var numOfFields = reader.readByte();
    var fields = <int, dynamic>{
      for (var i = 0; i < numOfFields; i++) reader.readByte(): reader.read(),
    };
    return User();
  }

  @override
  void write(BinaryWriter writer, User obj) {
    writer..writeByte(0);
  }
}

// registration
Hive.registerAdapter(UserAdapter())
@simc
Copy link
Member

simc commented Dec 29, 2019

I think this is a great idea. Thank you for the pull requests. Please give me a few days to merge them :)

simc added a commit that referenced this issue Dec 31, 2019
Added typeId property to HiveType (#163)
@simc
Copy link
Member

simc commented Jan 3, 2020

Published with v1.3.0.

@AlexV525
Copy link
Contributor

AlexV525 commented Jan 4, 2020

With this implement, I have something to say...
Once we moved typeId to annotation, it just makes the id management gets harder and chaos. Before I can wrote code like this:

int adapterIndex = 0;

Hive.registerAdapter(SomeAdapter(), adapterIndex++);
Hive.registerAdapter(AnotherAdapter(), adapterIndex++);

That makes the typeId completely auto increased with more and more adapter added. Same as when we are trying to add field (HiveField) in a HiveType. Users SHOULD NOT modify the sequence once adapters were registered.

The new change might made the typeId consist, but when there're ten or hundreds of HiveType need to be added, it required the project itself to build something like Map<Type, int> to find the actual typeId.

Too aggressive with this change, and it didn't land on a new major version. 😢

@simc
Copy link
Member

simc commented Jan 4, 2020

I would not recommend the way you registered TypeAdapters in the past. You already mentioned the first problem, you are not allowed to change the order. But how would you remove a type? It gets quite messy.

I'm thinking about a way to automate the typeId and fieldId management but it will take time.

Too aggressive with this change, and it didn't land on a new major version.

I'm sorry for that. I should have created a new major version.

Ultimately I think the new API is easier to use and more consistent. I also don't think it is a good idea to determine typeIds at runtime. They should be under version control.

@AlexV525
Copy link
Contributor

AlexV525 commented Jan 6, 2020

I would not recommend the way you registered TypeAdapters in the past. You already mentioned the first problem, you are not allowed to change the order. But how would you remove a type? It gets quite messy.

Actually when it comes to the app which was published, there's almost no chances to remove a type adapter or even a box.

I'm thinking about a way to automate the typeId and fieldId management but it will take time.

Maybe it just needs a box to store the ids :P (just kidding)

Now I'm using a class and constants to define typeIds, add or remove a type adapter will me more controllable and visualized:

@HiveType(typeId: HiveAdapterTypeIds.adapterA)
class A {}

class HiveAdapterTypeIds {
  static const int adapterA = 0;
  static const int adapterB = 1;
}

Generate constants was usually used to generate routes, I think at here it works.

@simc
Copy link
Member

simc commented Jan 6, 2020

Now I'm using a class and constants to define typeIds, add or remove a type adapter will me more controllable and visualized:

Great idea!!! Does the generator accept the constants? I might recommend that in the docs...

@AlexV525
Copy link
Contributor

AlexV525 commented Jan 6, 2020

Great idea!!! Does the generator accept the constants? I might recommend that in the docs...

Yes it works fine now. I've just finished reconfigure my projects with this migration. 😄

@MarcelGarus
Copy link
Contributor

MarcelGarus commented Jan 6, 2020

Sorry for not realizing this sooner, but this kind of broke my workflow.
In a project, I have some Entity classes, each of which has an Id<Entity>, which is just a typed wrapper class around a String. For example, NewsArticles would have an Id<NewsArticle>, Users an Id<User> and so on.

Because Hive doesn't support generics yet (writing an Id<User> with the auto-generated adapter and retrieving it returns an Id<dynamic>), I wrote a generic IdApater an then registered adapters for all the id types I use, like this:

class IdAdapter<T> extends TypeAdapter<Id<T>> {
  @override
  Id<T> read(BinaryReader reader) => Id<T>(reader.readString());

  @override
  void write(BinaryWriter writer, Id obj) => writer.writeString(obj.id);
}

class ColorAdapter extends TypeAdapter<Color> {
  @override
  Color read(BinaryReader reader) => Color(reader.readInt());

  @override
  void write(BinaryWriter writer, Color color) => writer.writeInt(color.value);
}
Hive
  // this is an excerpt of the registry process
  ..registerAdapter(IdAdapter<ContentType>(), 41)
  ..registerAdapter(IdAdapter<Content>(), 42)
  ..registerAdapter(IdAdapter<Course>(), 43)
  ..registerAdapter(IdAdapter<Lesson>(), 44)

This doesn't work anymore with this feature implemented.

@simc
Copy link
Member

simc commented Jan 6, 2020

You could do the following:

class IdAdapter<T> extends TypeAdapter<Id<T>> {
  @override
  final int typeId;

  IdAdapter(this.typeId);

  ...
}

Hive.registerAdapter(IdAdapter<ContentType>(41));
...

@MarcelGarus
Copy link
Contributor

Works for us this way, thanks 😊

For the future though, I would strongly advise against pushing breaking changes in minor versions. This goes against the principles of semantic versioning: Versions of the form MAJOR.MINOR.PATCH should only contain breaking changes in major releases, although @Deprecated annotations may be added in minor releases.
Because the pub package manager heavily relies on semantic versioning, builds may break. For example, for apps relying on hive: ^1.2.1 (which is equivalent to hive: '>= 1.2.1 < 2.0.0'), builds may succeed on the local machine if no pub get was run for some time, but builds fail on the server CI because the newest version of Hive is chosen.

simc pushed a commit that referenced this issue Feb 13, 2020
simc added a commit that referenced this issue Feb 13, 2020
Added typeId property to HiveType (#163)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants