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

Make standard codecs extensible #15414

Merged
merged 8 commits into from Mar 16, 2018

Conversation

Projects
None yet
3 participants
@mravn-google
Contributor

mravn-google commented Mar 11, 2018

Make standard message/method codecs extensible. Awaits companion flutter/engine PR (flutter/engine#4770).

Fixes #15389

});
} else {
throw new ArgumentError.value(value);
buffer.putUint8(_kUnknown);
writeUnknown(buffer, value);

This comment has been minimized.

@Hixie

Hixie Mar 12, 2018

Contributor

Another way to make this extensible would just be to have people override writeValue and call super.writeValue if they don't recognize the type.

This comment has been minimized.

@Hixie

Hixie Mar 12, 2018

Contributor

we'd have to define that the format is that you first push a byte then push the data, and define a range of bytes that we reserve for our purposes.

///
/// This method is intended for use by subclasses overriding
/// [readUnknown].
dynamic readValue(ReadBuffer buffer) {
if (!buffer.hasRemaining)
throw const FormatException('Message corrupted');
dynamic result;

This comment has been minimized.

@Hixie

Hixie Mar 12, 2018

Contributor

similarly here we could read out the first byte, then call readValue with that byte and the buffer, and they would override readValue and call super.readValue if they don't recognise the type

@Hixie

This comment has been minimized.

Contributor

Hixie commented Mar 12, 2018

My main concern with the readUnknown/writeUnknown thing is that it makes extensions into second class citizens rather than treating them the same way we treat our own code.

@mravn-google

This comment has been minimized.

Contributor

mravn-google commented Mar 12, 2018

My main concern with the readUnknown/writeUnknown thing is that it makes extensions into second class citizens rather than treating them the same way we treat our own code.

Roger that. My main concern was to protect the range of type discriminator bytes used by the base class. Documenting our way out of that makes for a more straightforward extension API. Done.

/// for values that the extension does not handle. Type discriminators
/// used by extensions must be greater than or equal to 128 in order to avoid
/// clashes with any later extensions to the base class.
void writeValue(WriteBuffer buffer, dynamic value) {

This comment has been minimized.

@Hixie

Hixie Mar 14, 2018

Contributor

maybe mention that it's ok to call this recursively, e.g. to implement writing a list of values.

@@ -201,6 +201,9 @@ class JSONMethodCodec implements MethodCodec {
/// `FlutterStandardTypedData`
/// * [List]\: `NSArray`
/// * [Map]\: `NSDictionary`
///
/// The codec is extensible by subclasses overriding [writeValue] and
/// [readValue].

This comment has been minimized.

@Hixie

Hixie Mar 14, 2018

Contributor

readValueOfType?

else if (value == 254)
return buffer.getUint16();
else
return buffer.getUint32();

This comment has been minimized.

@Hixie

Hixie Mar 14, 2018

Contributor

this might be more readable as a switch

@Hixie

This comment has been minimized.

Contributor

Hixie commented Mar 14, 2018

LGTM looks great.

mravn-google added some commits Mar 11, 2018

@mravn-google mravn-google force-pushed the mravn-google:make_stdcodec_ext branch from 10f227e to 1e4c9b2 Mar 16, 2018

@mravn-google mravn-google merged commit 340c680 into flutter:master Mar 16, 2018

4 checks passed

cla/google All necessary CLAs are signed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
flutter-build

@mravn-google mravn-google deleted the mravn-google:make_stdcodec_ext branch Mar 16, 2018

DaveShuckerow pushed a commit to DaveShuckerow/flutter that referenced this pull request May 14, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment