Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] useGenericObjectState: Fix types so that setPropValue function narrows value type by key #2994

Closed
wants to merge 1 commit into from

Conversation

mturley
Copy link
Contributor

@mturley mturley commented Jul 10, 2024

Standalone, semi-related to RHOAIENG-8454
Would resolve RHOAIENG-9748 if completed. Closing the PR for now because fixing the errors is out of the scope of the little "sneak in a tiny change" time I budgeted.

Description

I was helping @gitdallas look into using useGenericObjectState for RHOAIENG-8454 and I noticed it had this weird type quirk. I played around for a minute to see if it would be easy to fix and this very small fix worked fine, so I figured I should open a PR.

useGenericObjectState<T> returns (in part) a function of type UpdateObjectAtPropAndValue<T> which takes a key and value and updates the value at that key in the state object T. The current type of the argument propValue: ValueOf<T> means that no matter what propKey is passed in, TypeScript only enforces that your value is one of any of the valid property values for any key on the object. This means potential run time type errors, as I can see playing around with the usage in useRunFormData:

Screenshot 2024-07-10 at 5 55 42 PM

With this change applied, the type of propValue is narrowed to the actual type of the property matching the propKey you pass in.

Here's that same line with this change applied, correctly showing an error:

Screenshot 2024-07-10 at 5 56 44 PM

And if I change the key to the one that matches the value I'm passing, the error goes away:

Screenshot 2024-07-10 at 6 09 54 PM

⚠️ Note: this reveals a handful of type errors in the repo that should be fixed before this change is merged! I thought I was tossing in a quick little fix but I opened a can of worms here. I followed up by opening RHOAIENG-9748.

How Has This Been Tested?

Test Impact

Request review criteria:

Self checklist (all need to be checked):

  • The developer has manually tested the changes and verified that the changes work
  • Commits have been squashed into descriptive, self-contained units of work (e.g. 'WIP' and 'Implements feedback' style messages have been removed)
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has added tests or explained why testing cannot be added (unit or cypress tests for related changes)

If you have UI changes:

  • Included any necessary screenshots or gifs if it was a UI change.
  • Included tags to the UX team if it was a UI/UX change (find relevant UX in the SMEs section).

After the PR is posted & before it merges:

  • The developer has tested their solution on a cluster by using the image produced by the PR to main

@openshift-ci openshift-ci bot requested review from lucferbux and pnaik1 July 10, 2024 22:25
Copy link
Contributor

openshift-ci bot commented Jul 10, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from mturley. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@mturley mturley changed the title useGenericObjectState: Fix types so that setPropValue function narrows value type by key [WIP] useGenericObjectState: Fix types so that setPropValue function narrows value type by key Jul 10, 2024
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress This PR is in WIP state label Jul 10, 2024
@mturley
Copy link
Contributor Author

mturley commented Jul 10, 2024

Closing for now, we may want to reopen when we get around to addressing RHOAIENG-9748.

@mturley mturley closed this Jul 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge/work-in-progress This PR is in WIP state
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant