feat: add Edition 2023 Support#2051
Merged
sofisl merged 12 commits intoprotobufjs:masterfrom Apr 14, 2025
Merged
Conversation
714c8e1 to
c8c02e2
Compare
This will now work as long as all files sharing a package agree on file-level edition and options
c8c02e2 to
b2c6867
Compare
…nresolved options for fromJSON, fix aggregate feature handling
4088c3f to
a84409b
Compare
1bea8d6 to
f269dd1
Compare
f269dd1 to
ac9a3b9
Compare
sofisl
approved these changes
Apr 1, 2025
Contributor
sofisl
left a comment
There was a problem hiding this comment.
Mostly questions to determine whether something is breaking but seems mostly not.
sofisl
reviewed
Apr 1, 2025
Contributor
|
@dcodeIO can you take a look at this PR? Debating whether this should be a breaking change or not - would love your input, thanks! |
Merged
|
This broke presence detection for proto3 syntax, proto3 syntax only allows |
Tofandel
reviewed
Apr 24, 2025
Tofandel
reviewed
Apr 24, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Protobuf.js was audited for the following features:
The proto target CLI appeared to be broken in a number of edge cases related to editions features, so these were fixed and tested were added. It now supports proto2 and proto3 roundtrips, and conversions to proto2/proto3 (even from edition 2023) as long as they're valid. No extra validation layer was added, so impossible conversions will just produce invalid protos. A new target for edition 2023 wasn't added, but could be pretty easily if necessary in the future.
One major design flaw in protobuf.js, that's common in proto3-based code, is that the concept of files was largely dropped. The in-memory and JSON representation of descriptors puts all of the file metadata into the most-nested package namespace object, but this object may be shared by multiple files. This results in possible clobbering and merging, leading to malformed file metadata. In proto2/proto3, this largely doesn't matter outside of custom file options, but the effects can be seen in the hack that forces
packed = falsefor proto2 descriptors to get the repeated field encoding correct. This problem was made worse in editions, where we store important metadata at the file level (edition + features). The solution implemented here collapses all of this information into the top-level objects instead of putting it on the namespace. This results in some minimal duplication, but has the ability to properly support proto2, proto3, and editions in addition to fixing some pre-existing bugs (demonstrated by some newly added tests).Other changes:
packed=falseto make them behave properlyMigration Guide
.resolveAll()is called on dynamic built protos. This is needed when building from constructors directly or individual fromJSON/addJSON methods. Top-level builders likeRoot.fromJSON,Root.load, andprotobuf.parsedo not need to explicitly call thisBEGIN_COMMIT_OVERRIDE
feat: add Edition 2023 Support
END_COMMIT_OVERRIDE