Generate constructor arguments again, add a flag to disable #855
Conversation
How would you implement this? By patching the library internally, or with an env variable, or ...? |
Maybe by passing the option to not create the named parameters from the blaze-rule that compiles the protos? |
|
Jumping in to plus one this PR - I did encounter the usability difference in creating proto objects in a project I'm working on (dartpad uses protos to communicate between the frontend and backend). |
|
When will this be merged and published? This is needed ASAP. Thanks |
|
@sigurdm PTAL. |
|
Still LGTM. I would still prefer for the constructor arguments to be generated by default. I think that is the most user friendly and what most people should use. Google3 (and others) are free to turn them off if they really think it saves them anything significant. |
|
As a non-google user of this library I agree that having the constructors generated by default is preferable. As I understand, the only reason one would not want to have constructors is to reduce the final binary size. I cannot speak for the whole community, but I believe this optimization is not something many people worry about. The PR which removed constructors mentions the increase in binary size but with no concrete numbers, perhaps the file size increase is indeed significant? I also fear that people will simply not know about the constructor option and new users will just use the cascade operator construction. |
@sigurdm I don't think this makes sense for internal users. Internal users don't explicitly update their dependencies, so they won't check the changelog and see if they need migration. It's also not feasible to reach out to every single user, as there are just too many. So projects with binary size benchmarks will potentially be notified, and then they have to figure out what changed and how to revert the changes. Projects without binary size benchmarks will silently get larger. Also, no one needs this feature (it's not enabling anything new), and no internal user so far requested this feature even though it's been in the open source version for 2+ years (since Nov 2020). So there seems to be no benefit for the internal users, but their projects may get slightly larger. That said, I actually investigated a little bit why some projects get larger with these arguments. TFA is quite good at dropping unused arguments, and I've confirmed that VM and dart2wasm projects are not affected if they don't use the arguments. The problem is with dart2js projects, as dart2js doesn't use TFA and it doesn't seem to be dropping the arguments as aggressively. Something else I've considered is to make this the default in open source, and just modify one line internally: - _generateFactory(out);
+ out.println('factory $classname() => create();');But the problem is tests and golden files need to be different too, it's not as simple as just one line. This problem also exists if I somehow implement different defaults in internal and OS versions. |
|
We discussed this with @sigurdm and found a way to disable constructor arguments by default to internal users. I updated the PR to generate constructor arguments by default. |
osa1
changed the title
|
@sigurdm PTAL. |
| } | ||
| } | ||
| out.println('}) {'); | ||
| out.println(' final result = create();'); |
There was a problem hiding this comment.
Note that this local variable (result) can conflict with named params corresponding to a proto field.
For dartpad's protos code, we're seeing:
A value of type 'CompileResponse' can't be assigned to a variable of type 'String'.
Try changing the type of the variable, or casting the right-hand type to 'String'.
Perhaps name this _result? $result?
There was a problem hiding this comment.
Our proto definition:
message CompileResponse {
string result = 1;
string sourceMap = 2;
// Make this response compatible with BadRequest
ErrorMessage error = 99;
}
|
is it already included in a release ? how to check that ? |
|
This was included in version 21.1.0 |
|
i am running protoc --dart_out=dart ./proto/*.proto --proto_path=proto |
|
any update ? |
|
What version of |
|
Indeed the global protoc version I used was old. Now it works but |
Fixes #850.