jhomlala / alice

HTTP Inspector for Flutter. Allows checking HTTP connections with UI inspector.
544 stars 246 forks source link

feat: packages/alice_objectbox #205

Closed techouse closed 3 months ago

techouse commented 3 months ago

This PR adds the ability to store all the AliceHttpCalls in an ObjectBox Box via a package.

How to get started

  1. Add it to your dependencies
dependencies:
  alice_objectbox: ^1.0.0
  1. Follow the ObjectBox example
Future<void> main() async {
  /// This is required so ObjectBox can get the application directory
  /// to store the database in.
  WidgetsFlutterBinding.ensureInitialized();

  /// Initialize [AliceObjectBoxStore] before running the app.
  final AliceObjectBoxStore store =
      await AliceObjectBoxStore.create(persistent: false);

  /// Pass [AliceObjectBoxStore] to the app
  runApp(MyApp(store: store));
}

class MyApp extends StatefulWidget {
  const MyApp({
    super.key,
    required this.store,
  });

  final AliceObjectBoxStore store;

  @override
  State<MyApp> createState() => _MyAppState();
}
class _MyAppState extends State<MyApp> {
  late final AliceHttpAdapter _aliceHttpAdapter = AliceHttpAdapter();

  late final Alice _alice = Alice(
    showNotification: true,
    showInspectorOnShake: true,
    maxCallsCount: 1000,
    aliceStorage: AliceObjectBox(
      store: widget.store,
      maxCallsCount: 1000,
    ),
  )..addAdapter(_aliceHttpAdapter);

  // your custom stuff...
}

Caveats


Addresses #156

jhomlala commented 3 months ago

I've merged UI refactor PR. Please rebase :)

techouse commented 3 months ago

@jhomlala I think this is ready for review. Please check/run the provided example in examples/alice_objectbox.

Note that I made some modifications to AliceCore, mostly related to variable and method visibility. Where I made these modifications I converted private variables and methods to their @protected equivalents. Please let me know if any of the changes pose any risks.

techouse commented 3 months ago

Good job! I think we could make it even better with some changes. What do you think? I'm happy to help you with that.

I've made some changes based on the suggestions given and left comments where I think they either can't be done or they don't make much sense imho.

Please, do play around with the example. Maybe I've missed something :)

jhomlala commented 3 months ago

Good job! I think we could make it even better with some changes. What do you think? I'm happy to help you with that.

I've made some changes based on the suggestions given and left comments where I think they either can't be done or they don't make much sense imho.

Please, do play around with the example. Maybe I've missed something :)

I've copied your work and applied my abstractions for storage. I think it will be cleaner. The default storage will be memory one - nothing really has changed here. I've modified your new package to expose only new storage which can be injected during Alice configuration.

Please take a look and let me know what do you think. You've done a lot of work with ObjectBox, so I could easily refactor it with abstraction. https://github.com/jhomlala/alice/tree/feat/alice_storage_abstraction

techouse commented 3 months ago

I've copied your work and applied my abstractions for storage.

Ah, I've just invited you to my fork so you wouldn't have to copy stuff all over the place 😀 Looks like I was too slow

techouse commented 3 months ago

Your fork is missing the implementations of _getNotificationMessage and _showLocalNotification.

Also, why is this now a method and mapped 3x instead of 1x?

@override
  Stream<List<AliceHttpCall>> getCallsStream() {
    return store.httpCalls
        .query()
        .order<int>(CachedAliceHttpCall_.createdTime, flags: Order.descending)
        .watch(triggerImmediately: true)
        .map((Query<CachedAliceHttpCall> query) => query.find())
        .map((calls) {
      return calls.map((call) {
        return call.getAliceHttpCall();
      }).toList();
    }).asBroadcastStream();
  }

Not sure why this is needed. What's wrong with this?

Stream<List<AliceHttpCall>> get callsStream => _store.httpCalls
      .query()
      .order<int>(CachedAliceHttpCall_.createdTime, flags: Order.descending)
      .watch(triggerImmediately: true)
      .map((Query<CachedAliceHttpCall> query) => query.find())
      .asBroadcastStream();

It's a stream after all.

jhomlala commented 3 months ago

AliceCore from alice package will handle that. AliceStorage has void subscribeToCallChanges(Future<void> Function(List<AliceHttpCall>? calls) callback); which is being used in AliceCore to listen to the changes.

techouse commented 3 months ago

I'll try to implement all the changes from your fork into mine. ⏳

jhomlala commented 3 months ago

Also, why is this now a method and mapped 3x instead of 1x?

@override
  Stream<List<AliceHttpCall>> getCallsStream() {
    return store.httpCalls
        .query()
        .order<int>(CachedAliceHttpCall_.createdTime, flags: Order.descending)
        .watch(triggerImmediately: true)
        .map((Query<CachedAliceHttpCall> query) => query.find())
        .map((calls) {
      return calls.map((call) {
        return call.getAliceHttpCall();
      }).toList();
    }).asBroadcastStream();
  }

Not sure why this is needed. What's wrong with this?

Stream<List<AliceHttpCall>> get callsStream => _store.httpCalls
      .query()
      .order<int>(CachedAliceHttpCall_.createdTime, flags: Order.descending)
      .watch(triggerImmediately: true)
      .map((Query<CachedAliceHttpCall> query) => query.find())
      .asBroadcastStream();

It's a stream after all.

Yeah it's dirty solution, just wanted to show some PoC. Feel free to remove duplicated code.

techouse commented 3 months ago

@jhomlala OK, I've merged all your stuff with mine and I think it now looks good.

Run over it once you can and let me know it you have more feedback.

jhomlala commented 3 months ago

I don't want to block this PR, it looks good, but I'm thinking about unit tests (at least for storages) and updating docs. What do you think? Thank you so much for this hard work.

techouse commented 3 months ago

Unit testing storage will be quite labor intensive, as you have to create a local ObjectBox on the runner, i.e. a macOS binary etc. It's all well documented on ObjectBox's website ... but still a PITA to set up. 🙈

I'd rather make driver tests, but then you have to run simulators, compile the thing and play it on a sim in Github Actions + mock an HTTP server because you don't wanna DDoS anyone 🤪 #fun

The documentation is for the most part there in the example. Not sure if there should be any more readmes etc. I can add some more stuff, if needed.

jhomlala commented 3 months ago

Sure, I understand. We can do it in separate PR. I'm thinking about doing unit tests for things that we can do, for example memory storage. For hard stuff - we can do integration tests.

For documentation I was thinking about adding info about storage here: https://github.com/jhomlala/alice/blob/master/docs/config.md.

Well I think we can merge it, but I keep looking for some tests - we're getting more and more logic.

techouse commented 3 months ago

I added some more docs.

jhomlala commented 3 months ago

@techouse Are you still working on this PR?

techouse commented 3 months ago

Just fixed a bug 🐛 should be all good now

techouse commented 3 months ago

Hmm, I was just thinking whether all the add / remove etc methods should be async. Waiting for them synchronously is quite a pain, i.e.

FutureOr<void> addCall(AliceHttpCall call);

FutureOr<void> addError(AliceHttpError error, int requestId);

FutureOr<void> addResponse(AliceHttpResponse response, int requestId);

FutureOr<void> addHttpCall(AliceHttpCall aliceHttpCall);

FutureOr<void> removeCalls();

Maybe in a new PR. This one is too big already :D

techouse commented 3 months ago

@jhomlala let me know if you're happy with this PR. Then we can start tackling other stuff.