Add WithResources to shared components #1636
Conversation
openshift-ci-robot
added
the
size/L
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: jtomasek If they are not already assigned, you can assign the PR to them by writing The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@jtomasek: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
| if (resource) { | ||
| if (resource.loaded) { | ||
| childrenProps[resourceKey] = resource.data; | ||
| } else if (resourceConfig.required) { |
There was a problem hiding this comment.
Would it make sense to do small refactoring when already moving the code?
I like isRequired more, as it is boolean.
| const { onError, loaderComponent, children } = props; | ||
|
|
||
| React.useEffect(() => checkErrors(errors, onError), [errors]); | ||
| React.useEffect(() => setResourcesState(getResourcesState(props)), [props]); |
There was a problem hiding this comment.
What is the second argument [props] for, please?
There was a problem hiding this comment.
IIUC, getResourcesState(props) will result in the same value as for the useState() default above. Can you please elaborate this flow?
There was a problem hiding this comment.
Second argument is the array of values that the effect depends on, the effect gets run only when one of the values changes. When an empty array is passed, the effect is only run on mount (same as componentDidMount). (https://reactjs.org/docs/hooks-reference.html#useeffect).
Original implementation used static getDerivedStateFromProps function to populate the state from props. First the state was initialized in constructor:
this.state = Resources.getDerivedStateFromProps(props);. To mimic this I am defining the useState hook and initialize the state by calling the function getResourcesState(props).
Then to update the state on props change I am using useEffect hook which gets run only when props change.
|
We're introducing a second component like |
I tend to agree, I am introducing this because a lot of components from kubevirt/web-ui are using it. We'll need to take a look at how big the impact is converting those to use Firehose directly. |
|
FYI we've attempted to implement a new |
Right, understood. I did notice |
|
@spadgett We chatted about this briefly today with @jelkosz and @rawagner and there's an agreement to improve existing
I agree 👍 |
|
@jtomasek , I think we can close this already, right? |
Yes, this change is no longer needed. Closing. |
As described here: #1539 (comment)
WithResourcesis an utility which simplifies work with Firehose and adds some features:Firehosewont render children if model does not exist in k8s.WithResourceswill - both cases has their placeResourcesaccepts resourceMap which is object instead of array as in Firehose which brings us a few enhancements:resourceMapkeys are used as firehose resource prop which makes it easier if there is more queries for the same model, object use enforces use of unique keysresource from
resourceMapcan be flagged asrequired- if yes we wait with child rendering until the resource is loaded (or we can show loading)there is also way to define your own error handling
no need to check Firehose's loaded prop - when injected resource is undefined it means it is still loading