Skip to content

web: correctly coerce RuntimeStatus string to proper type#1749

Merged
jazzdan merged 2 commits into
masterfrom
dmiller/ch2750/initial-builds-don-t-show-error-circle
Jun 14, 2019
Merged

web: correctly coerce RuntimeStatus string to proper type#1749
jazzdan merged 2 commits into
masterfrom
dmiller/ch2750/initial-builds-don-t-show-error-circle

Conversation

@jazzdan

@jazzdan jazzdan commented Jun 14, 2019

Copy link
Copy Markdown
Contributor

No description provided.

@jazzdan
jazzdan requested a review from hyu June 14, 2019 15:52
@jazzdan
jazzdan requested review from landism and removed request for hyu June 14, 2019 16:17
Comment thread web/src/status.tsx
return RuntimeStatus.Error
}
return runtimeStatus

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bit confusing to me... My best guess - is the intent of this change to make it so that if we add a new status and don't update this, we convert it to Unknown? Or is this where the actual coercion is happening and I don't understand TS enums?

also, ffti code golf (well, maybe slightly better than code golf since it's a bit more DRY-y, or maybe slightly worse since it's using case fallthroughs):

case RuntimeStatus.Error:
case RuntimeStatus.Pending:
case RuntimeStatus.Ok:
  return status
default:
  return RuntimeStatus.Unknown

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, the Resource typo only uses primitives since we can't know the type of what we get from the server (everything on the internet is a string).

Right now that conversion happens here but we should probably have a step further up where we validate/add types to the websocket response.

@jazzdan
jazzdan merged commit e1ec68c into master Jun 14, 2019
@nicks
nicks deleted the dmiller/ch2750/initial-builds-don-t-show-error-circle branch February 3, 2022 16:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants