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

Remove event support #880

Merged
merged 6 commits into from
Oct 12, 2023
Merged

Remove event support #880

merged 6 commits into from
Oct 12, 2023

Conversation

osa1
Copy link
Member

@osa1 osa1 commented Oct 10, 2023

Event support is currently unused internally and we don't want to introduce new
uses of it, to keep the API surface small and the library easier to update, and
to make it easier to migrate to another library.

createRepeatedField and createMapField existed to support events (as
PbList and PbMap don't support this), so with the event mixin removed we
remove these members as well.

Removing these members give us other opportunities: we now have full control
over the field value types. This allows, for example, refactoring PbMap and
PbList types for marking them as frozen without visiting the elements, which
makes it possible to implement decoders that create frozen objects without
having to make another pass after decoding to mark every object as frozen.

Closes #738.

cl/571893384

This removes `PbEventMixin`, `PbFieldChange`, and `EventBuffer` types,
and the event plugin mixin support from the plugin.

This mixin is currently not used internally and we don't want to
introduce uses to it to keep the API surface small, to make it easier to
make changes or migrate to another protobuf library.

Fixes google#738.

cl/571893384
protoc_plugin/CHANGELOG.md Outdated Show resolved Hide resolved
@osa1 osa1 requested a review from sigurdm October 10, 2023 08:02
@@ -21,9 +21,6 @@ const GeneratedMessage_reservedNames = <String>[
'clone',
'copyWith',
'createEmptyInstance',
'createMapField',
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we should leave these? Otherwise the names will no longer be disambiguated breaking existing code?
Not sure what is better here...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you suggest we keep them indefinitely, or remove later?

If you want to remove later, this is already a breaking change so we could do it now.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah - it is either remove

  • now or
  • never.

Not sure what is better. We could check if we have any usages inside google3. These names are probably quite rare - so maybe it is better to remove now.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested the CL globally internally it doesn't break anything.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, in that case I vote for just removing them

@osa1 osa1 merged commit 96d9522 into google:master Oct 12, 2023
17 of 18 checks passed
@osa1 osa1 deleted the remove_event_stuff branch October 12, 2023 15:29
osa1 added a commit to osa1/protobuf.dart that referenced this pull request Oct 26, 2023
osa1 added a commit that referenced this pull request Oct 26, 2023
osa1 added a commit to osa1/protobuf.dart that referenced this pull request Nov 22, 2023
This improves iteration performance by making `moveNext` and `current`
direct calls in kernel. google#902 does not have the same effect on `moveNext`
and `current` calls, this change is needed even with google#902.

This change was not possible before google#880 as users could override the
list and map types to types that are not `PbList`s or `PbMap`s.
osa1 added a commit that referenced this pull request Nov 23, 2023
This improves iteration performance by making `moveNext` and `current` direct
calls in kernel. #902 does not have the same effect on `moveNext` and `current`
calls, this change is needed even with #902.

This change was not possible before #880 as users could override the list and
map types to types that are not `PbList`s or `PbMap`s.

Note: changes in the message field types (the plugin changes in this PR) are
not necessary for the kernel improvements, but it doesn't hurt to generate more
precise types everywhere.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove event mixin stuff
2 participants