[App Search] Minor var names follow-up #87258
Conversation
- so it doesn't sound negative or like a bug, but is instead an anticipated load/use case - remove unncessary function - move declaration down to right before it's used
- Rename engineNameParam to engineNameFromUrl to make reading flow a little bit more nicely + hopefully make the var source a bit clearer - Change other references back to engineName for simplicity (they should really only be running after engineName has already been set, so there shouldn't be any race conditions there - moving engineBreadcrumb to after Loading should also solve that)
cee-chen
added
release_note:skip
| @@ -69,31 +69,30 @@ export const EngineRouter: React.FC = () => { | |||
| }, | |||
| } = useValues(AppLogic); | |||
|
|
|||
| const { engineName: engineNameFromUrl } = useParams() as { engineName: string }; | |||
There was a problem hiding this comment.
Feel free to push back if you disagree, but I was thinking FromUrl flowed a little better reading-wise and also made it a little clearer where we're setting engineName from. I also played with engineNameFromParam but ended up likeing FromUrl a little better
| defaultMessage: "No engine with name '{engineNameParam}' could be found.", | ||
| values: { engineNameParam }, | ||
| defaultMessage: "No engine with name '{engineName}' could be found.", | ||
| values: { engineName }, |
There was a problem hiding this comment.
There isn't really a realistic scenario where engineName would get set after engineNotFound gets set so I think it's safe to simplify this
| const isLoadingNewEngine = engineName !== engineNameFromUrl; | ||
| if (isLoadingNewEngine || dataLoading) return <Loading />; | ||
|
|
||
| const engineBreadcrumb = [ENGINES_TITLE, engineName]; |
There was a problem hiding this comment.
Similarly, I wanted to simplify engineBreadcrumb to just use engineName (and also to think of engineName as a source of truth once an engine is initialized from url), so I moved this to after the Loading component to reduce confusion & keep the logic together.
cee-chen
changed the title
cee-chen
changed the title
💚 Build SucceededMetrics [docs]Async chunks
Distributable file count
To update your PR or re-run it, just comment with: |
* Var rename - so it doesn't sound negative or like a bug, but is instead an anticipated load/use case - remove unncessary function - move declaration down to right before it's used * [Proposal] Other misc cleanup - Rename engineNameParam to engineNameFromUrl to make reading flow a little bit more nicely + hopefully make the var source a bit clearer - Change other references back to engineName for simplicity (they should really only be running after engineName has already been set, so there shouldn't be any race conditions there - moving engineBreadcrumb to after Loading should also solve that)
Summary
Follow up to #86702 (comment): a var rename suggestion and then a little more because I can't leave well enough alone
Checklist