No error is returned or thrown when chart fetching fails.

[martvalja]

Chart fetching may fail for multiple reasons: database connection lost, chart is not found from the database etc. Currently null is returned and there might be some logging, but no good way to handle it in the code.

I had two ideas how to handle this: 1) Return object that contains chart and/or error 2) Throw error

First one seems like a better option to me, what do you think?

---

Edit: With brief look at the code I do not know where exactly it might have failed in such a way that it returns null even though there is a row in the database. It seems that xjog does not catch errors so those should bubble up, but it might be that some higher order functions eat those errors, await/async + higher order functions behave in mysterious ways sometimes.

[samihult]

Sorry for late response.

I think the standard solution is to have an optional callback and return a promise, if the callback has not been provided. It's idiomatic and both ways convey the same information.

getChart(charId: string , cid?: string): Promise<chart>;
getChart(charId: string , cid?: string, cb: (err, chart) => void): void;

If there is a problem that is difficult to identify, turning on trace-level logging can help. There is a lot of logic, and as you noted, problems don't really bubble up. One notable case is a deadlock, which are not completely avoidable.

Some opinions and thoughts about the implementation, in reference to the PR that was probably meant for the fork:

1. Don't throw if the case is normative, i.e. return null if an entity is not found by the persistence layer instead of throwing. It's not an error and, imo, does not deserve an exception. E.g. sending an event to the void is logically not a problem, it just does not do anything. This is an important philosophical aspect of statecharts – they can or can not react to events – and I would like to extend that to a system of statecharts as well.
2. The PR added getChartMachineId, which I don't think is necessarily a great idea. The primary key (in PG) is ("machineId", "chartId") – different machines can have charts by the same id and this is by design. Rather store the machineId with the chartId. You can use e.g. URI:s for that (see XJog.getChart). On the other hand, if you use XJogMachine.getChart, you already know the machine id. It's not part of this issue, but mentioning, because I cannot approve this change.
3. The callback form is not in any way important. Throwing JS errors is not that expensive, and callbacks have big drawbacks. That's why promises were brought along in the first place.

[martvalja]

Main issue is that it is currently not possible to know programmatically if chart fetching failed or if chart does not exist (for example, it has been deleted from database for whatever reason).

1. When subscribing to a chart to show it to the user, we need to know if chart exists but loading failed or chart does not exist in the database as behaviour should be different in each case. Currently I do not think it is possible to determine that.
2. That is interesting, I was not aware of this. We added this mainly to get rid of code that tries machine A and if chart is not found, tries machine B etc, as it has worked from beginning for second and third level charts that have multiple options. But that has also then the same issue that there is a possibility for multiple matches.
3. I personally prefer to use explicitly returned errors (in golang style) to throwing errors or using callbacks. But I agree that throwing error is simpler.

[samihult]

Easiest first:

3) JS has its conventions and I think it's wise to follow them 2) I get the use case, but would recommend using URIs then (they probably did not exist back in the day) 1) I would return null if it doesn't exist, and if there is a real error, throw one