Publishing rest messages - android only #2
Conversation
- moving few classes from `ably.dart` to `src/spec.dart`
As we don't need them to be implemented as pure dart methods. Must be mixed with platform channel calls using PlatformObject.
- move necessary implementations from `src/ably_implementation.dart` to `src/impl/*.dart` - use `ably.dart` only to export other files - move code from `able.dart` to `interface.dart` and `defaults.dart` - update example code - update tests
- This will be required by multiple method calls as messages need to be casted for each method call
- spec refactored. classes in spec will be implemented wherever plugin functionality is necessary. - classes in spec can be implemented in pure dart library (purely based on IDL and some java references) - lib/src/impl will have all plugin related implementations - example code updated with ably rest instance creation and publishing messages to a channel
…ler enhancement - LibraryException renamed to AblyException - AblyException takes code and message as positional arguments in that order
- codec moved to separate file - TokenDetails included in codec - bytecode for AblyMessage is changed to max available i.e, 255 thereby giving it least preference and accommodate serialization other classes with lower code - new `createRestWithKey` in Ably, for simpler instance creation - Added few doc strings
Will be used for tests
platform messages posted to dart will be decoded by `readValueOfType`
- default log level will be info - new platform tests for rest implementation (creation of instance) - remove print statement from PlatformObject.invoke
There was a problem hiding this comment.
Hello @tiholic!
Thanks - it looks like you've moved this quite a bit forward. 😁
I would certainly still expect, at this early stage, for these functional increments to be unstable and incomplete. So please don't feel you have to excuse yourself, though I would certainly encourage you to do your best to either:
- Always submit iOS and Android together as a single atomic increment alongside, obviously, the supporting Dart code.
- Make sure you submit iOS soon after Android so as to, apart from anything else, not lose your contextual advantage at that time. And, also, there's a chance that once you get on to iOS that you'll realise you need to refactor all the way up to the Dart again if something doesn't quite work the same way that it does in Android.
Going forward I would appreciate it if you could please try to keep the PRs as small as possible. In light of what I've just written above, this means that I'm asking you to find a comfortable size of functional increment that balances 'complete' (as defined in Agile terms) with simplicity/complexity for those reviewing. I'm happy to have a chat if you need me to expand more on what I mean here.
Thanks again. And please do keep feeding back to @mattheworiordan (and/or me) in respect of how you feel plugin development is going. We are particularly keen to be aware of anything you encounter that might affect the developer experience for Flutter developers encountering our plugin. You are there, at the coalface, so best positioned to detect and communicate issues or concerns - in particular those that might become blockers or usability constraints.
|
Thanks @QuintinWillison Noted! Will try to keep iOS code updated in line with android. And sure, will keep you and Matthew in loop about anything could become a challenge to developers using this plugin. |
There was a problem hiding this comment.
Thanks @tiholic.
Great to see you are getting your head around it all and making progress.
I agree with Quintin's comments. One other thing I noticed is divergence from our IDL, which I'd like you to check at https://docs.ably.io/client-lib-development-guide/features/#idl as that is the only source of truth about the spec. I appreciate it's hard as I sent you the TypeScript IDL, which is a slight divergence from the spec as JS is not 100% compatible.
| static const _valueAblyMessage = 255; | ||
|
|
||
| @override | ||
| void writeValue (final WriteBuffer buffer, final dynamic value) { |
There was a problem hiding this comment.
I can't helping this can be DRY'd up somehow. Is this necessary to hard code every attribute like this, when these definitions exist in other places like the type specs?
There was a problem hiding this comment.
@mattheworiordan multiple reasons
- This part is specific to plugin while spec can be used by pure dart library
- Same order will be followed in Android/iOS code, so this will be a one place reference for all transferrable datatypes
@QuintinWillison your comments, please.
| return t; | ||
| case _valueAblyMessage: | ||
| return AblyMessage( | ||
| readValue(buffer) as int, |
There was a problem hiding this comment.
My comment in a previous commit about DRY'ing this up, seems even more necessary when I see this duplicated here.
|
P.S. Before this is merged, please ensure |
https://github.com/ably/ably-flutter/pull/2/files?file-filters%5B%5D=.dart&file-filters%5B%5D=.gradle&file-filters%5B%5D=.iml&file-filters%5B%5D=.java&file-filters%5B%5D=.properties&file-filters%5B%5D=dotfile#r410713469 https://github.com/ably/ably-flutter/pull/2/files?file-filters%5B%5D=.dart&file-filters%5B%5D=.gradle&file-filters%5B%5D=.iml&file-filters%5B%5D=.java&file-filters%5B%5D=.properties&file-filters%5B%5D=dotfile#r410713596
https://github.com/ably/ably-flutter/pull/2/files?file-filters%5B%5D=.dart&file-filters%5B%5D=.gradle&file-filters%5B%5D=.iml&file-filters%5B%5D=.java&file-filters%5B%5D=.properties&file-filters%5B%5D=dotfile#r410713469 https://github.com/ably/ably-flutter/pull/2/files?file-filters%5B%5D=.dart&file-filters%5B%5D=.gradle&file-filters%5B%5D=.iml&file-filters%5B%5D=.java&file-filters%5B%5D=.properties&file-filters%5B%5D=dotfile#r410713596
https://github.com/ably/ably-flutter/pull/2/files?file-filters%5B%5D=.dart&file-filters%5B%5D=.gradle&file-filters%5B%5D=.iml&file-filters%5B%5D=.java&file-filters%5B%5D=.properties&file-filters%5B%5D=dotfile#r410710920 Example updated to use environment instead of restHost or realtimeHost https://github.com/ably/ably-flutter/pull/2/files?file-filters%5B%5D=.dart&file-filters%5B%5D=.gradle&file-filters%5B%5D=.iml&file-filters%5B%5D=.java&file-filters%5B%5D=.properties&file-filters%5B%5D=dotfile#r410714630
|
@tiholic I've added some comments and responded to some of the questions. |
QuintinWillison
force-pushed
the
integration/plugin
branch
from
062fa58 to
ffa0572
Compare
Channels class is abstracted by a base channels class which is extended by Rest and Realtime each restricting their own Channel classes The changes mentioned in below comment are also accommodated #2 (comment)
- `.fromKey` and `.fromOptions` are removed from high level interface #2 (comment) - Getting rid of XXXBase and XXX as it is only required for ably-java since it supports plain java and android #2 (comment)
- Getting rid of XXXBase and XXX as it is only required for ably-java (since it supports plain java and android) #2 (comment)
- `.fromKey` and `.fromOptions` are removed from high level interface #2 (comment) - Getting rid of XXXBase and XXX as it is only required for ably-java since it supports plain java and android #2 (comment)
- Getting rid of XXXBase and XXX as it is only required for ably-java (since it supports plain java and android) #2 (comment)
- `.fromKey` and `.fromOptions` are removed from high level interface #2 (comment) - Getting rid of XXXBase and XXX as it is only required for ably-java since it supports plain java and android #2 (comment)
- Getting rid of XXXBase and XXX as it is only required for ably-java (since it supports plain java and android) #2 (comment)
- `.fromKey` and `.fromOptions` are removed from high level interface #2 (comment) - Getting rid of XXXBase and XXX as it is only required for ably-java since it supports plain java and android #2 (comment)
- Getting rid of XXXBase and XXX as it is only required for ably-java (since it supports plain java and android) #2 (comment)
- `.fromKey` and `.fromOptions` are removed from high level interface #2 (comment) - Getting rid of XXXBase and XXX as it is only required for ably-java since it supports plain java and android #2 (comment)
- Getting rid of XXXBase and XXX as it is only required for ably-java (since it supports plain java and android) #2 (comment)
Hoping to solve this error in workflow environment: Unhandled exception: Invalid argument (uri): Unknown package: Instance of '_SimpleUri' #0 _resolveUri.<anonymous closure> (package:dartdoc/src/generator/resource_loader.dart:34:9) #1 _RootZone.runUnary (dart:async/zone.dart:1612:54) #2 _FutureListener.handleValue (dart:async/future_impl.dart:152:18) #3 Future._propagateToListeners.handleValueCallback (dart:async/future_impl.dart:704:45) #4 Future._propagateToListeners (dart:async/future_impl.dart:733:32) #5 Future._completeWithValue (dart:async/future_impl.dart:539:5) #6 Future._asyncCompleteWithValue.<anonymous closure> (dart:async/future_impl.dart:577:7) #7 _microtaskLoop (dart:async/schedule_microtask.dart:40:21) #8 _startMicrotaskLoop (dart:async/schedule_microtask.dart:49:5) #9 _runPendingImmediateCallback (dart:isolate-patch/isolate_patch.dart:120:13) #10 _RawReceivePortImpl._handleMessage (dart:isolate-patch/isolate_patch.dart:185:5) Error: Process completed with exit code 255.
Spec creation from reference typescript IDL
Inspiration drawn from existing codebase and ably-java implementation.
Upgrade gradle and Ably java library.
AblyException(this will be used wherever appropriate in the library for throwing meaningful exceptions)