Add back named constructor arguments (or include as an option)

[agreaves]

All the recent changes and compatibility with Dart 3 are phenomenal! So first of all thanks for that!

However, removing constructor arguments is an incredibly harmful change for code quality. These constructors provide much needed structure and encapsulation. Given Dart's formatting, the cascading Foo()..bar = 4 approach is unwieldily and leads to significantly less readable code. The example given in the original issue illustrates this well.

In our case, this is a barrier to upgrading our proto libraries. We'd have to undergo a painful migration just to have worse code quality in the end. In the meantime, we'll likely either stay on the old version or fork the repo and revert this change (which will not be fun either).

For us, the slightly increased binary size is a negligible side effect of having an easy way to construct messages. However, at the very least, I'd request making it configurable via an option (defaulting to false if you consider it prudent).

Thanks again for this awesome library! I think more people should be using it, and this would make it more developer friendly 🙂

[agreaves]

In addition, one other extremely important function of constructor arguments is the ability to pass in null values (based on some condition). To use the example I mentioned in the original, you could previously do:

Person(
  name: givenName,
  middleName: hasMiddleName ? middleName : null,
  favoriteDinosaur: isNoFun ? null : dinoSelection,
)

This you could accomplish in one encapsulated statement, and means it can be used in many contexts (e.g. passing it in as an argument). Now, the equivalent would be:

final person = Person()..name = givenName;
if (hasMiddleName) {
  person.middleName = middleName;
}
if (!isNoFun) {
  person.favoriteDinosaur = dinoSelection;
}

which is both far less readable and far less convenient.

[Levi-Lesches]

To add to this, my team uses Protobuf to sync data between C++, Python, and Dart. We use protobuf messages alongside regular classes in Dart and draw no distinction between the two. When we want to sync some data to the other platforms, we just transfer the declaration from Dart to Protobuf.

This change means we'll have to track which classes are written in Dart and which are generated by Protobuf in order to use them correctly. It also means that changing a class to a message is a much more involved process, where we have to change every time it is constructed. We'd much prefer the current way, with the constructor arguments, even if it results in a slightly larger executable. Keep in mind that flutter already produces a fair bit of bloat, which we are more than okay with.

[Pante]

Besides what has already been mentioned above, this change is particularly harmful in the case of nested message.

Given:

class Top {
  Top({this.a, this.b, this.c});

  Middle? a;
  Middle? b;
  Middle? c;
}

class Middle {
  Middle({this.a, this.b, this.c});

  Bottom? a;
  Bottom? b;
  Bottom? c;
}

class Bottom {
  Bottom({this.a, this.b, this.c});

  int? a;
  int? b;
  int? c;
}

The following:

final top = Top(
    a: Middle(
      a: Bottom(a: 1, b: 2, c: 3),
      b: Bottom(a: 1, b: 2, c: 3),
      c: Bottom(a: 1, b: 2, c: 3),
    ),
    b: Middle(
      a: Bottom(a: 1, b: 2, c: 3),
      b: Bottom(a: 1, b: 2, c: 3),
      c: Bottom(a: 1, b: 2, c: 3),
    ),
    c: Middle(
      a: Bottom(a: 1, b: 2, c: 3),
      b: Bottom(a: 1, b: 2, c: 3),
      c: Bottom(a: 1, b: 2, c: 3),
    ),
  );

will become:

  final top = Top()
    ..a = (Middle()..a = (Bottom()..a = 1..b = 2..c = 3)..b = (Bottom()..a = 1..b = 2..c = 3)..c = (Bottom()..a = 1..b = 2..c = 3))
    ..b = (Middle()..a = (Bottom()..a = 1..b = 2..c = 3)..b = (Bottom()..a = 1..b = 2..c = 3)..c = (Bottom()..a = 1..b = 2..c = 3))
    ..c = (Middle()..a = (Bottom()..a = 1..b = 2..c = 3)..b = (Bottom()..a = 1..b = 2..c = 3)..c = (Bottom()..a = 1..b = 2..c = 3));

This forces us to choose between two equally poor choices, continue using method chaining at the cost of readability, or refactor the code to use local variables which makes the code significantly more verbose.

[khomin]

oh that is why it stopped working
any advice how to dart pub global activate protoc_plugin with last working version?

i found it

dart pub global deactivate protoc_plugin
dart pub global activate protoc_plugin 20.0.1

[almovsesanso]

oh that is why it stopped working any advice how to dart pub global activate protoc_plugin with last working version?

i found it

dart pub global deactivate protoc_plugin
dart pub global activate protoc_plugin 20.0.1

Thanks for the helpful command. It's ridiculous that they've decided to break existing code just like that without even trying to make it a parameter or something.

[t-kozak]

Assuming that the command above is correct (I trust it, just haven't checked) and knowing that the latest version of dart protoc is 20.0.2, our friends here introduced large backwards API incompatibility when increasing the version only by 0.0.1.

API breaking changes like this has to be planned long in advance with deprecation and not simple removal with a bump of a minor library version.

[osa1]

our friends here introduced large backwards API incompatibility when increasing the version only by 0.0.1

This change was introduced with a major version bump from protoc_plugin-20 to 21.

[JYeop]

I think the breaking change (v20 -> v21) is making the regression of the code readability.
Quoting the upper response @agreaves made,
Nested object to be almost unreadable if the params are getting more and more.

Getters and setters are useful when the class parameters are changing.
I understand the grpc has default values and we are re-assigning them.
But virtually, We are making a new object to use the grpc api.
I don`t know what is right to use.
But the code readability side, i think the named-constructor is way better solution.

final top = Top()
    ..a = (Middle()
      ..a = (Bottom()..a = 1..b = 2..c = 3)
      ..b = (Bottom()..a = 1..b = 2..c = 3)
      ..c = (Bottom()..a = 1..b = 2..c = 3))
    ..b = (Middle()
      ..a = (Bottom()..a = 1..b = 2..c = 3)
      ..b = (Bottom()..a = 1..b = 2..c = 3)
      ..c = (Bottom()..a = 1..b = 2..c = 3))
    ..c = (Middle()
      ..a = (Bottom()..a = 1..b = 2..c = 3)
      ..b = (Bottom()..a = 1..b = 2..c = 3)
      ..c = (Bottom()..a = 1..b = 2..c = 3));

final top = Top(
    Middle(
      Bottom(1, 2, 3),
      Bottom(1, 2, 3),
      Bottom(1, 2, 3),
    ),
    Middle(
      Bottom(1, 2, 3),
      Bottom(1, 2, 3),
      Bottom(1, 2, 3),
    ),
    Middle(
      Bottom(1, 2, 3),
      Bottom(1, 2, 3),
      Bottom(1, 2, 3),
    ),
);

[marianhlavac]

I'd like to express the same opinion, this update seems really half-baked and breaks codebases in a spectacular way.

It hurts the code quality the most when using optional fields, with a perfect example from @agreaves .

Please reconsider this decision, as we'll probably have to stick with version <21, which is not ideal.

[osa1]

Thanks for the feedback. We're adding a flag to generate old style code with constructor arguments in #855, so no need to add more use cases/examples here.

[Levi-Lesches]

Just want to take a moment to thank the team here, especially @osa1 for that PR. I'm sure even frustrated users understood this was just a change made too quickly, and I'm really happy that it was resolved. In fact, my team hadn't even updated the package since the removal, and now we won't even feel the issue at all.

[RubenGarcia]

How do you fill repeated fields? I don't see a setter in the generated code, and my protoc does not have the new option.
Never mind
#8
explains it.