-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
feat!: add Edition 2023 Support #2051
base: master
Are you sure you want to change the base?
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
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.
Mostly questions to determine whether something is breaking but seems mostly not.
var field = oneof.fieldsArray[0]; | ||
|
||
return field.options != null && field.options["proto3_optional"] === true; | ||
function isNullable(field) { |
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 seems like a breaking change - seems that syntax no longer determines nullability, just whether the field has presence & not required?
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.
It's really just a refactor and simplification. All of the syntax logic has been encapsulated into feature helpers like hasPresence and required (see https://protobuf.dev/editions/implementation/#syntax_reflection), and feature resolution itself.
This should be logically equivalent for proto2/proto3, but is now compatible with editions also.
this._valuesFeatures[key] = Object.assign(parentFeaturesCopy, this._valuesProtoFeatures[key] || {}); | ||
} | ||
this._valuesFeatures[key] = Object.assign(parentFeaturesCopy, this.valuesOptions && this.valuesOptions[key] && this.valuesOptions[key].features); | ||
}); |
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 isn't equivalent to previous, seems like we're assigning a boolean vs. an object.
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.
How so? this is just using similar short-circuit logic with &&
instead of ||
. Basically, if there are features for the value already, merge them, otherwise assign null (i.e. copy parentFeaturesCopy)
@@ -176,21 +173,6 @@ Enum.prototype.add = function add(name, id, comment, options) { | |||
if (this.valuesOptions === undefined) | |||
this.valuesOptions = {}; | |||
this.valuesOptions[name] = options || null; | |||
|
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.
trust you, but we needed this to have feature-level enums
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.
Yea I collapsed all protoFeatures directly into options in a cleanup commit to reduce the footprint of the PR. So the unresolved features get stored like regular options now, and resolved features get tracked separately
@dcodeIO can you take a look at this PR? Debating whether this should be a breaking change or not - would love your input, thanks! |
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 = false
for 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=false
to 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.parse
do not need to explicitly call this