-
Notifications
You must be signed in to change notification settings - Fork 18
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
Removing catch around parse function, and result types #111
Comments
Or are there certain scenarios where you found that |
Attempting to parse partial data was one case: #46 I think there are other reasons to do this, though. Are there tradeoffs besides the cognitive overhead of having to deal with results in some parts of the code? |
would partial data be typed as nullable anyway? Or perhaps we should have a annotation that data might be partial that gets picked up by the ppx (so all types are nullable)? How does partial data work in Apollo? Is it on a fragment basis? |
It looks like when returning partial data everything is fair game for apollo. So basically to be able to support this, all fields need to be optionals. So a top level query directive like |
That does seem like a better solution to a partial data scenario. Here are a few other examples of when parse might raise an exception:
In general my thought is that I never trust anything coming into the system even if it's supposed to work. I personally use Apollo Client in situations where it's absolutely critical that we perform some other action if something were to fail, so this just in general felt like the right tradeoff to me. Could we discuss more what the pain is you're feeling around this? |
You'll read a fragment/query from the cache, right? These should have the same guarantees.
Yes, but the GraphQL protocol should never break in these scenarios as it is backwards compatible. Usually you'll mark fields that are going to disappear as deprecated first. I work on a native app, with old versions around, so this is always a concern. BTW if you do things like drop fields or something like that, your app is going to break in some way anyway. Crashes are better IMO, because it would trigger alerts (and the server can be fixed quickly).
In these scenarios a crashing app is not that bad. It's more easily diagnosed, and it's exceptional behavior.
Hmm that is strange. Authorization should be separate from the schema. Also changing schemas at runtime is not really compliant with the spec I believe. Do you have an example?
But is an exception not better than a silent error?
GraphQL is a protocol, so it does have guarantees about some things. If you are not going to trust it, why would you type it? In certain exceptional situations you can do you own exception handling? (same as in JS?) |
Perhaps we can have vanilla hooks that do not do parsing on the data and work with the However working with the |
Haha, I think we're just rehashing the exact same discussion we had around promises. I think a function that returns a result should always return a result and you think exceptional circumstances should be an exception. I obviously find the experience of using results to be superior in all the scenarios where you describe exceptions superior. I'm not quite sure why we're not able to easily align on this issue. I do, however, trust you greatly (more than myself) as a programmer. I don't know that there is a "right" answer either. Both have tradeoffs, IMO. If you have some passion to make this change, let's do it!
Not going to break down each of the other points, but I felt maybe it's worth clarifying my definition of "trust" here. I wouldn't say I trust it unless I have no exception handling (absolute 100% trust). And if I need exception handling, I think those exceptions should be accounted for in the result. This lack of "trust" doesn't invalidate the usefulness of the types in my eyes.
I think I would lean toward choosing one path and keeping things simple. |
Well you shouldn't do that haha
Yes. The problem with trying to deal with exceptional circumstances, you'll end up with overly defensive programming and complexity starts to increase exponentially. There is a cost to dealing with results. Code becomes less readable. So I think in general it should only be done when there is a failure that is expected. I recently used a codebase that used results everywhere, it's not pretty. And refactoring it moving some exceptional cases to exceptions made the code way more readable (also easier to debug btw, because you know where the crash happened).
I think we should compare this to JS or TS. You don't extract the code that accesses the data that comes back from GraphQL, and wrap it in a Thinking more deeply. What are the specific things we are catching?
I think all other cases would not raise an exception in the parse stage (if the type stages it wouldn't raise an exception, it would just mistype the result). This could also be fixed with the thing we were talking about earlier? (Making all fields nullable in case of delivering partial results). |
Regardless, you put a ton of energy into the community, graphql-ppx, and this library. If we can change the ergonomics of the library for the better for you, that’s a pretty big force in favor. It would be different if anyone else contributed 😄
I've also been in codebases that used results heavily and found the experience to be really great. I wonder why our experiences are so different. I’m going to continue with the discussion because I’m curious. But before I do, I’ll state that while I would personally stick with what we have since it feels correct to me from a philosophical perspective, I know it has tradeoffs and isn’t even really addressing a problem that is likely to occur. I’m perfectly fine going in this direction if you feel strongly we should. But it’s still not clear to me what problem we’re trying to solve here, so even if it’s the better direction, I cannot say if it’s worth unwinding the try/catch 😅 Since we’re talking about results in general, I’ll try to give my perspective on their usage in general before moving on to whether this should throw an exception. I think all of us that have dealt with results realize it is a rabbit hole where you quickly reach the conclusion that every single function could possibly throw an exception, so all functions become results returning. We can probably all agree that is a path to madness? Instead we adopt more of a “functional core, imperative shell” style of programming where everything is checked at the edges of the program and results are constrained only to things that interface with the outside world. When limited in scope to the edges of the program, I really don’t find results to be adding much complexity, and the argument of added complexity isn’t swaying me very much. In particular, we’re already using a result and will continue to do so in this case? Could you tell me a bit more about how the argument against results in general applies here if we’re still going to use a result? I think I would understand if the argument were to go back to a straight In a typical scenario, let’s pretend our possible responses can be represented like so:
Sure, it’s JS. But I don’t want a JS? I want something more along the lines of an elm-lang that has escape hatches and great react support. Doing all of this checking at the edges has costs, but I’ve always felt them worth paying. Maybe it’s just me, but in a functional language, results are sacred. Returning a result implicitly states to me, “I’m not going to throw an exception”. This mixing and matching is extremely unexpected and breaks some fundamental invariant I rely on.
Now that you mention it, it’s clear I was operating on assumptions from what I do think the community seems much more aligned with your way of thinking, though. I used Thoughts? |
Actually I agree with most of what you are saying. I am only of the opinion that GraphQL is a typesafe protocol, and once you query the schema there is a contract between the client and the server that is the GraphQL schema. If we accidentally made this change (we were not aware that this field was already used in the app when we made the breaking change), I would say a hard crash is good. Because this problem will be caught in QA. If we fail silently and not crash it's harder to find and debug the error. Actually If we want to support partial results, this should be properly typed in I also have some issues with exceptions being swallowed in my application potentially somewhere in |
We don't do full checking of the JSON because the number of checks we need to do used to generate an exponential number of permutations in the generated code, which blew up code size and hurt performance quite a bit for larger/deeper queries. See this issue. |
I'm aligned on the GraphQL part at this point, thanks for bearing with me! But if we return to the original issue I referenced about partial data, it was never the desire to get partial data, it was an unintentional situation because there were errors. In that issue (or in a linked So, we don't get to just drop the try/catch, right? Are you basically proposing we swap the try/catch around parse for an if/else checking for errors? What exactly do we gain by doing that? You seem to be worried mostly about exceptions being swallowed and silent failures? Is this the main issue we're trying to solve for at this point? |
Um I think I didn't realize the impact. If we are parsing partial data (whether it's due to an errorPolicy or with the partialResults option), we should have the types and the parse function that |
Perhaps we can have a |
so if you don't have the annotation, it will fail to compile because |
BTW the partialData story is interesting. I saw that Apollo is going to have a more precise partial rendering technique in 4.0 that is more similar to Relay with |
I don't have a ton of clarity at this point how we make things typesafe for
I have an irrational level of fear of this. Well, that and the dreaded uncaught
Interesting! I have not been following Apollo all that closely of late. |
Yes. But errorPolicy is not typesafe right now as well (most type errors will happen outside of parse). So the only way to make this typesafe is again to make everything in the query nullable. Which is the same thing you need to with partial data. BTW to be able to do this, there needs to be a change to the
👍 |
Yeah, that's clear to me. I think what I'm really getting at with my comment about a lack of clarity, is that I would wait to remove the catch until we have a strategy in place to either partially parse or not parse when appropriate. It just seems unfortunate in instances like that initial issue I referenced that had the partial data, we have someone trying to track down why their app is crashing and not understanding the nuances. As it stands, we would preserve the authentication error, and even give you the data at the same time if we can parse it, as opposed to obscuring the real cause behind a type error and crashing the app. Are you proposing we remove it now even without the ppx additions? |
No we have to wait for the But just saying that with the current approach it crashes mostly outside of the parse function, because most of the time the parse function just does a dumb type coercion based on the known schema. So the crash would be happening when trying to access a field that does not exists. |
👍 |
There is some defensive programming in the client where there is an expectation that the parse function can fail. GraphQL is a protocol with certain guarantees. That means we can assume data shapes and that in turn means that the parse function would never raise an exception unless there is a bug in
graphql-ppx
.Can we remove these checks?
The text was updated successfully, but these errors were encountered: