Skip to content
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

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

Open
brototyp opened this issue Apr 8, 2020 · 10 comments
Labels
enhancement New feature or request floret:inspector The Inspector Floret proposal Proposing a new feature.

Comments

@brototyp
Copy link
Member

brototyp commented Apr 8, 2020

Problem

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

  • the status code is pretty much always 200
  • the path never changes
  • the method is always GET

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.

@brototyp brototyp added enhancement New feature or request proposal Proposing a new feature. floret:inspector The Inspector Floret labels Apr 8, 2020
@pstued
Copy link
Member

pstued commented Apr 8, 2020

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
Copy link
Member Author

brototyp commented Apr 8, 2020

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
Copy link
Member

pstued commented Apr 8, 2020

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
Copy link
Member Author

brototyp commented Apr 8, 2020

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
Copy link
Member

pstued commented Apr 8, 2020

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
Copy link
Collaborator

Shukuyen commented Apr 8, 2020

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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Collaborator

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
Copy link
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request floret:inspector The Inspector Floret proposal Proposing a new feature.
Projects
None yet
Development

No branches or pull requests

3 participants