Check if glyphs are available

[maxammann]

This PR adds two things

• Suggestion: A new result type which is in my opinion way better than throwing exceptions
• Code for checking glyphs

I would like to refactor the web frontend soon with the Result type

[steffenkleinle]

Like it 👍 Not tested.

[steffenkleinle]

Is this used anywhere? Perhaps you should wrap the whole thing in a try catch and return this err on catch?

[maxammann]

I think this is worth discussing. There can always be unexpected crashes, but I dont think we should just add try catches for errors we do not expect.

[steffenkleinle]

But then this is currently unused and could/should be removed, right?

[maxammann]

oh sorry, yes, I forgot to use this error type. I planned to use it around the .output() function :) thanks!

[michael-markl]

I do like the Rust way of handling errors as well, but I do not think, that it should be applied to JS too much. We will never get rid of try .. catch blocks anyways as we use third party libraries. The problem here is, that JS has throw baked in and it is widely used. Therefore, if we use the Rust way here, we will get some strange mix of error handling throughout the code which is never appreciable.

Also I do think that we should (at some points) catch any error in a predefined process, so the rest of the app does not crash and go blank and instead it shows some nice error message, that something went wrong. (However this can also be handled at some global place, I guess).

If you want to catch predefined errors you can simply create a new Error Type and throw and catch a specific error type. (Of course you do not have the nice, exhaustive handling that Rust has that way). We could however add jsdoc's @throws documentation to these functions (or is that already outdated?)

We can leave it for the PDF process. However note that if e.g. jsPDF has some internal error, we will (probably) leave the user with an unusable, broken app. (or there might be no feedback to the user pushing the "Print PDF" button).

Edit: I also did not test it.

[maxammann]

I do like the Rust way of handling errors as well, but I do not think, that it should be applied to JS too much. We will never get rid of try .. catch blocks anyways as we use third party libraries. The problem here is, that JS has throw baked in and it is widely used. Therefore, if we use the Rust way here, we will get some strange mix of error handling throughout the code which is never appreciable.

Also I do think that we should (at some points) catch any error in a predefined process, so the rest of the app does not crash and go blank and instead it shows some nice error message, that something went wrong. (However this can also be handled at some global place, I guess).

If you want to catch predefined errors you can simply create a new Error Type and throw and catch a specific error type. (Of course you do not have the nice, exhaustive handling that Rust has that way). We could however add jsdoc's @throws documentation to these functions (or is that already outdated?)

I agree with all of your points, but my conclusion is different, because the alternatives are very bad. Adding a new error type results in the following code:

try {
  myroutine(); // There's a couple of errors thrown here
} catch (e) {
  if (e instanceof TypeError) {
    // A TypeError
  } else if (e instanceof RangeError) {
    // Handle the RangeError
  } else if (e instanceof EvalError) {
    // you guessed it: EvalError
  } else if (typeof e === "string") {
    // The error is a string
  } else if (axios.isAxiosError(e)) {
    // axios does an error check for us!
  } else {
    // everything else  
    logMyErrors(e);
  }
}

(https://fettblog.eu/typescript-typing-catch-clauses/)

The way I see it is that after 1-2 years of development nobody knows which error can happen when calling any function.
I even forgot which error classes we have in the Integreat api-client or in the Integreat app :D
By introducing a Result type early on this is clearly documented in the signature of a function. I would suggest that we never throw in our application and always use Result. If we call external libraries then we should check the libraries documentation whether it can fail. If it can fail, then handle the errors and wrap them in a Result. This should avoid the "mix of error handling throughout the code".

While this is a non-default JS way of handling errors, this should work fine in an application. Library developers should never do something like that. They should stick to the language default.

I agree that we should avoid the app getting blank or anything like that. This can be achieved by Error Boundaries: https://reactjs.org/docs/error-boundaries.html
Uncought errors in onClick etc. will go silent nonetheless.

[maxammann]

I think the important thing is that we do it the same everywhere in the administration app. Either use Error subtypes or use the custom Result. What do you prefer?

[michael-markl]

I dont have a strong preference between both methods.
However, in this particular case, I'd like to see some fallback catching internal errors of jsPDF, as in the complexity of PDF creation there might always occur unexpected errors.

I think we can hook ourselves into the global onError callback and trigger the AppToaster?

[maxammann]

I dont have a strong preference between both methods. However, in this particular case, I'd like to see some fallback catching internal errors of jsPDF, as in the complexity of PDF creation there might always occur unexpected errors.

I think we can hook ourselves into the global onError callback and trigger the AppToaster?

I think that will end badly 🙈
I would only do this if it is documented and allowed by the spec to overwrite for example console.error
Yes when calling third party libraries I would always wrap it in a try-catch if I would suspect that it is "failable".

[maxammann]

I think we can hook ourselves into the global onError callback and trigger the AppToaster?

There is an event handler for this: https://developer.mozilla.org/en-US/docs/Web/API/GlobalEventHandlers/onerror#window.addeventlistenererror

I think this needs maybe some trial and error to gain some experience under which circumstances this is called. The documentation is not that clear about it imo.

[michael-markl]

I think it's pretty well documented here. It is (for example) called when a js runtime error is thrown (and not caught anywhere).
I think you only have to be careful not to create/throw an error inside the onError handler to avoid an endless loop of death^^

[maxammann]

Lets decide this on Thursday: Legacy JS way vs. Results

[michael-markl]

@maxammann "Legacy JS way" is not really legacy 😅

[maxammann]

@maxammann "Legacy JS way" is not really legacy 😅

Maybe its the wrong word, lets use traditional/usual

[maxammann]

@klinzo @Schedulaar what do you think about this? It uses the traditional error handling method, combined with a "sum type" using a TS switch-case

[michael-markl]

A TS switch-case is exhaustive? If that is the case, then I disagree with the current solution, as long as you don't rename Exception to PDFException or so. Otherwise, we'd have to include a path for every PDF-Generation-Kind-Of-Error at any part in the app where we would catch errors, right?

[maxammann]

A TS switch-case is exhaustive? If that is the case, then I disagree with the current solution, as long as you don't rename Exception to PDFException or so. Otherwise, we'd have to include a path for every PDF-Generation-Kind-Of-Error at any part in the app where we would catch errors, right?

It does not force you to handle all cases, but suggests it it autocomplete.

[michael-markl]

Ah, alright 👍🏽

[maxammann]

I would just suggest to use this error class (the Exception class) whenever possible :) probably will try my best during the refactoring