cauliframework / cauli

Debug Networking
https://cauli.works
MIT License
28 stars 5 forks source link

Add an option to customize the representation of a record in the InspectorRecordTableViewCell #207

Open brototyp opened 4 years ago

brototyp commented 4 years ago

Problem

When using GraphQL, the current way of showing the records in the list is pretty meaningless, because:

Proposal

  1. Add a RecordFormatter protocol roughly like this:
protocol RecordFormatter {
    func canFormat(record: Record) -> Bool
    func method(for record: Record) -> String
    func status(for record: Record) -> String
    func time(for record: Record) -> String
    func path(for record: Record) -> String
}
  1. Add a DefaultRecordFormatter implementing that protocol with the current behavior.

  2. Add a parameter to the InspectorFloret initializer where we can pass in the formatter. That could be either the formatter directly, or a configuration object. We can then pass the formatter along to the InspectorTableViewController and us it to format the record in the cell.

Options

We can either leave it at this point and let users of Cauli implement their formatter, or add a GraphqlRecordFormatter to Cauli and automatically switch between both.

pstued commented 4 years ago

Looks nice and straight forward. I'm just wondering if it makes sense just to implement a GraphQLInspectorFloret itself? Unfortunately I'm not that experienced with GraphQL itself and wonder if there are more "edge/special" cases upcoming which we rather should handle in an own floret.

brototyp commented 4 years ago

That's another interesting approach and yeah, I guess the question then is ... how different do we need the GraphQlInspector. On the other hand, this proposal then would not just add the option to customize the list for GraphQL, but also for other kind of patterns. I don't know, let's say XML-RPC or such. I think having a very flexible way of formatting the list could help in general, right?

pstued commented 4 years ago

I think having a very flexible way of formatting the list could help in general, right?

Could help and/or make it just unnecessarily complex.

But I agree, having this kind of protocol gives us flexibility. We still could hide this logic in the framework itself and just provide a GraphQlInspector or make it public to expose it outside of the framework for people to implement their own ideas.

brototyp commented 4 years ago

Hm. I see your point. One more downside with two different florets for that would be that, say we have 5 different kind of network transports, there would be 5 different kind of inspectors. Wouldn't it be better to have just one where all of the requests + responses show up? Or am I just making up scenarios? 🤔

pstued commented 4 years ago

I think you have a valid point there.

Or am I just making up scenarios? 🤔

Not sure, I was not even expecting to have a second kind of Inspector. Shouldn't an Inspector just present the records? And that pretty straight forward. Maybe we should rethink how you store GraphQL data?

@Shukuyen what's your feeling about our conversation?

Shukuyen commented 4 years ago

There are good arguments for both solutions. I have a tendency towards a unified inspector to reduce complexity for future fixes and improvements (for example if we improve the search we would need to modify the default and the graphql inspector to benefit from the improvement - or abstract the base inspector functionality away, which would be a more complicated variant of the RecordFormatter protocol.

One more downside with two different florets for that would be that, say we have 5 different kind of network transports, there would be 5 different kind of inspectors.

That would also be the case with the solution proposed here, if we do not allow different protocols for different network requests/responses :-)

Unfortunately I'm not that experienced with GraphQL

I am unfortunately still in the same situation. @brototyp maybe you can mock up what you would like to see in the inspector for GraphQL? That could help us understand better and find a good (and simple?) solution.

brototyp commented 4 years ago

I am unfortunately still in the same situation. @brototyp maybe you can mock up what you would like to see in the inspector for GraphQL? That could help us understand better and find a good (and simple?) solution.

Honestly, I don't 100%ly know what I would like to see in the inspector for gql, and maybe that's one of the reasons why I wrote this proposal, since it doesn't really say how it should look like, merely that the current situation doesn't work.

Having that said, yes I think we should really talk about what exactly we need in the case of gql. I'll mock up an initial proposal from where we can take the discussion. Thanks for that idea!

brototyp commented 4 years ago

Alright, here is my first draft.

The right box shows how all GraphQL requests currently look like in Cauli. The only thing that is different is actually the time.

The green box shows what my proposal would be. Instead of the status code, we show if there was any error in the request (success / failure). Instead of the Method we show if it was Query or a Mutation and instead of the path/domain we show the name of the query or mutation. In theory we could add the parameters of a query or mutation, but I would say it's fine to just do that on the next level once you drill down on a single request.

What do you think?

GraphQLLike

Shukuyen commented 4 years ago

Definitely makes sense to have some other view on that kind of data. I am not closer to a decision which approach would be the best, though.

If there was a way to detect GraphQL requests automatically, I would say let's integrate it in the inspector. Otherwise I would lean to a GraphQL inspector floret, which would have the advantage over a user customizable protocol that there was something working out of the box.

Shukuyen commented 4 years ago

Found this project that could be used for inspiration: https://github.com/Ghirro/graphql-network

They detect GraphQL by the Content-Type and if there is a query parameter. Doesn't sound that solid for all cases, so if we integrate the GraphQL functionality in the InspectorFloret and do detection that way, there should be a toggle to disable this.