-
Notifications
You must be signed in to change notification settings - Fork 197
Split protobuf package into libraries #1026
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
Conversation
In preparation for adding conditional imports to be able to use different encoding/decoding implementation based on the platform, this PR creates an "internal" library and exports some of the important types for encoding/decoding like `FieldInfo`, `PbFieldType` (renamed as `PbFieldTypeInternal`). This syncs some of cl/613649886.
This reverts commit 32911d8.
devoncarew
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change looks good from the POV of a general refactoring. My main question is what the actual impact on the public API surface area is. If you know it off the top of your head, and do not have concerns, then sgtm. Otherwise, you might look into:
- that API diff tool from BMW
- running
dart docbefore and after the change, and manually inspecting the output - try a small, api => normalized markdown tool I wrote a while ago, and see how that works; the results are intended to be something you'd diff. https://github.com/devoncarew/app_dirs/blob/main/tool/api.dart
|
@devoncarew I compared the API before and after these set of PRs using apitool. Somehow it seems to generate lots of false positives. I've pasted the full diff it generates below. You'll see a lot of lines like: and things like: I don't know why it generates these changes.. When I manually get rid of the false positives the remaining changes are: I also don't understand what it means by "Interface FieldSet added". When I document the library I can't find a public type named Both the library and plugin needs a major version bump with these changes. @devoncarew do we want to merge these all together, or as separate PRs? full apitool output |
|
I just reverted the renaming because I think it's not necessary (see comments above). This should be backwards compatible now. (apitool still reports lots of false positives...) |
I have not used apitool myself; I think what it does is impressive but am not surprised that it gets tripped up on some things. Re: the renaming and "the type was already public, but it's a mysterious undocumented type that (I hope) no one's using". I suspect that is wasn't used by users but was used by the generated code. I believe you're correct above that to ship this rename we'd need a major version bump for both packages. This is something we can do if we want to keep in sync w/ internal; I think a revert here (which you've done) and look into reverting the renaming internally is also something we can do and probably less friction.
Separate PRs seems fine to me. |
devoncarew
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
|
Since protoc_plugin did not change at all (other than a test file where we have file paths for |
|
cc @devmil
|
|
thanks for the hint @mosuem |
This is the first part of the PRs that sync internal JSON decoding chages.
To be able to conditionally import different JSON decoders, this splits the
monolithic protobuf package into libraries.
These parts are now internal libraries:
These changes are not useful on their own, they're to prepare the library for
the rest of the PRs and to keep the PRs small and easier to review.