search

InseeFrLab / onyxia

🔬 Data science environment for k8s
https://onyxia.sh
MIT License
432 stars 78 forks source link

Cannot load MyService.tsx component when monitoringUrl is undefined #804

Closed slim0 closed 1 month ago

slim0 commented 2 months ago

Hi,

We updated onyxia with the 8.16.4 version. In our configuration, the monitoringUrl is undefined. In that case, the UI (MyService.tsx component) is not loaded properly. We dig in a bit and it seems that it came from this assert line:

https://github.com/InseeFrLab/onyxia/blob/720f8ff64e716fe30426bf7565b739f1503c8cd7/web/src/core/usecases/serviceDetails/selectors.ts#L137

Question: Why asserting something that can be undefined? Maybe some other variables are also impacted.

garronej commented 2 months ago

Sorry about that, and thanks for reporting.

In this particular case, using assertions enables enhanced type safety. It allows TypeScript to understand that if isReady is true, then helmReleaseFriendlyName, podNames, and selectedPodName are not undefined. I was a bit quick in my assertion, forgetting that monitoringUrl can be undefined in this region. I should probably use null to represent "not yet fetched" instead of using undefined. It would prevent me from making this kind of mistake again.

More generally, assertions tell TypeScript to "trust me, I know what I'm doing" in cases where the type system cannot follow. Whenever an assertion is incorrect, like here, it makes for a very easily traceable error.

fcomte commented 1 month ago

@garronej is it fixed ? i guess yes

garronej commented 1 month ago

Yes it is.

I'll do a small refactor so this kind of issues may not happen again. Ref #816