ui/ServiceLink: convert to typescript #2853
Conversation
web/src/app/links/ServiceLink.tsx
Outdated
| export const ServiceLink = ( | ||
| service: Service | null | undefined, | ||
| ): JSX.Element => { | ||
| return <AppLink to={`/services/${service?.id}`}>{service?.name}</AppLink> |
There was a problem hiding this comment.
Service is nullable and potentially undefined in its use in AlertDevice
Handling the nullability/undefined here results in potentially invalid or dead links. Is there a preferred method here? Could always conditionally render the AppLink 🤔
EDIT: I meant conditionally render the Grid it contains entirely
goalert/web/src/app/alerts/components/AlertDetails.tsx
Lines 372 to 376 in a9d8684
The JavaScript variant didn't handle these, so it's likely this never happened, but could happen.
There was a problem hiding this comment.
Yeah, I think the ServiceLink component should require it to be non-null as it's impossible to render a link to a service without its info. The JS version would have just thrown an error and broken the page, so the type info should reflect its requirement.
The call site rendering has a potential bug; conditionally rendering it is fine.
That said, I think having it undefined and throwing an error would have broken the page, so it's likely the data is guaranteed by some previous loading check anyhow. If that is the case, there might be a way to have the type information reflect that it's not nullable in practice.
There was a problem hiding this comment.
Went up to the Page level and found the assertion being made on alert being present
I couldn't find an assertion on service being present, the nullability/undefined is being handled
From here, conditionally rendering makes the most sense to me 👍
There was a problem hiding this comment.
Conditional rendering done in 42dd136
The props are destructured and renamed to alert. Cleaner to use that instead of referencing the data directly.
In the AlertDetails component ServiceLink is now conditionally rendered to prevent a situation where the service is null/undefined and rendering a link to the service.
| serviceID={props.data?.service?.id ?? ''} | ||
| serviceID={alert?.service?.id ?? ''} |
There was a problem hiding this comment.
Super minor refactor: above the props are destructured and renamed to alert
| {ServiceLink(alert.service)} | ||
| </Typography> | ||
| </Grid> | ||
| {alert.service && ( |
There was a problem hiding this comment.
From my understanding of JS, the && should be fine here since Objects are never falsy
docs: https://developer.mozilla.org/en-US/docs/Glossary/Falsy
this should avoid the common gotcha with && and conditional rendering. Since undefined and null won't render and alert.service is always truthy
https://reactjs.org/docs/conditional-rendering.html#inline-if-with-logical--operator
make checkto catch common errors. Fixed any that came up.Description:
Part of an ongoing effort to convert existing javascript files to typescript.
Which issue(s) this PR fixes:
#2318