[Discuss - frontend best practice] one way data flow

[Bobgy]

https://reactjs.org/docs/refs-and-the-dom.html It's recommended to avoid using ref when possible, because the data/event flow mechanism conflicts with React's recommendation of one-way data flow philosophy (see https://reactjs.org/docs/thinking-in-react.html).

For example, instead of exposing open() and close() methods on a Dialog component, pass an isOpen prop to it.

Similar to this example, I think to avoid exposing refresh() as a public method, we could have a prop called sth like refreshCounter: number, the child components can do a refresh whenever it sees an update in refreshCounter -- using componentDidUpdate or useEffect react hook. In this way, we no longer need to add refs all the way and trigger a method to refresh components, one refresh would be as simple as a state update to the refreshCounter. We'd also need a onLoad callback, that will be triggered by child components when a page finishes loading.

After avoiding using ref to get component instances, we can write components using functions and react hooks: https://reactjs.org/docs/hooks-overview.html. It's a nice way to make abstraction on state changes (and a lot more!).

_Originally posted by @Bobgy in https://github.com/kubeflow/pipelines/pull/5040#discussion_r569956232_

[Bobgy]

/cc @zijianjoy @StefanoFioravanzo

[Bobgy]

Another partially related problem is the Page abstraction: https://github.com/kubeflow/pipelines/blob/1f32e90ecd1fe8657c084d623b2fcd23ead13c48/frontend/src/pages/Page.tsx#L40-L99

Recap of what problem the Page abstraction tries to solve

This is a standard KFP page:

Some UI elements are common:

• title
• back button
• breadcrumbs
• buttons in toolbar

In order to reuse this design and logic for all KFP pages, they are built as root of router in https://github.com/kubeflow/pipelines/blob/1f32e90ecd1fe8657c084d623b2fcd23ead13c48/frontend/src/components/Router.tsx#L230.

This is a reasonable choice, because in the DOM tree, these items are close to the root. So callbacks to control these common elements are passed using the Page interface to each page in a route.

Several problems with Page

• Page uses inheritance (deprecated) to reuse some common logic and build a common interface.
• Callbacks to control global state: updateToolbar, updateBanner are passed to Page. The interface for controlling toolbar state is imperative, rather than declarative. After each state update, we need to call updateToolbar in a child component. e.g. _selectionChanged handler.
• Tests used Page interface implementation details pervasively, e.g. https://github.com/kubeflow/pipelines/blob/1f32e90ecd1fe8657c084d623b2fcd23ead13c48/frontend/src/pages/ExperimentDetails.test.tsx#L412-L415 and https://github.com/kubeflow/pipelines/blob/1f32e90ecd1fe8657c084d623b2fcd23ead13c48/frontend/src/pages/ExperimentDetails.test.tsx#L385 Therefore, it's a lot of work to refactor these components, all tests that use these implementation details need to be refactored at the same time.

Why it's still like this?

Because

• the efforts needed to refactor (including the tests) is huge
• the implementation sort of scales OK for KFP current use-cases
• the problems they incur are more of aesthetic & tech debt than productivity (at least until today)

Therefore, I have never thought a refactoring is of enough priority, considering the number of other more meaningful ways we can improve KFP.

[Bobgy]

How I would have implemented it?

From my past experience, more idiomatic way of implementing these is:

• Build standard layout & atomic components for the common page features. A layout component can accept either render props or react element, so that what's rendered inside the layout can still be determined by page component. These components should better be stateless and controlled by their parents.
• Let each page component use these common elements to implement pages.
• Build page component tests using react testing library, rather than accessing component instance and methods. (as documented in https://github.com/kubeflow/pipelines/issues/5118#issuecomment-776177682)

Benefits:

• All the elements are easily reusable and composable.
• Page component rendering logic can be declarative (e.g. which buttons exist on the page can be derived from state of the page, instead of controlled imperatively via callback).

Note, I think these only apply to Banners, Toolbars, Breadcrumbs and Title. Snackbar and dialogs might need to be rendered across pages, so they are probably not a good fit.

Plan

If we agree on the ideas, what we can do is building new pages using this more idiomatic paradigm. I still don't think it's worth it to rewrite the entire KFP UI just for this refactoring. If there are other reasons we need to build/rebuild pages, that will be good chances to start applying the new practices.

[zijianjoy]

Thank you for the detailed explanation of the concept! I agree with using render props for reusable and portable components on the common UI elements on the top of pages. One question: The UI elements listed are unified across different pages, except buttons in toolbar. Each page will have different set of buttons, and based on user action on children element, these buttons might change on the same page. How does each layer look like if we apply render props for button elements?

[Bobgy]

@zijianjoy I built a quick demo: https://codesandbox.io/s/magical-violet-hwfxb?file=/src/App.js

Notice how the Layout component defines layout of the page, while App component can inject stuff into the Layout component dynamically using either render prop or react elements as props.

So that, back to KFP UI, we can define a common PageLayout component that gets reused across each Page, and the pages can control what buttons to render in layout slots.

[zijianjoy]

@Bobgy Thank you so much Yuan for building the sample demo!

To confirm if I understand it correctly, here is a simplified example for KFP UI - RunList:

<Page>
     <ExperimentDetail>
            <PageLayout />
            <RunsList>
                  <CustomTable />
           </RunList>
     </ExperimentDetail>
</Page>

In the above layout, RunsList determines what buttons to show (for example, Archive/Activate/Clone runs), and CustomTable determines which buttons are enabled/disabled because of user selection, and RunsList collects run selection callback from CustomTable. ExperimentDetail Collects button information from RunsList callbacks, and updates PageLayout accordingly. In such case, can I assume that ExperimentDetail determines what buttons should show in PageLayout? Because ExperimentDetail collects all button related information from children, and use render props to render PageLayout.

As reference, the Page component looks like below:

  render() {
    return (
      <div>
        <ExperimentDetail render={buttonState => (
          <PageLayout buttonState={buttonState} />
        )}/>
      </div>
    );
  }

[Bobgy]

     <ExperimentDetail>
            <PageLayout />
            <RunsList>
                  <CustomTable />
           </RunList>
     </ExperimentDetail>

Page is unnecessary, files in the pages folder are already pages.

The following understanding has a key difference, because button management can be very dynamic, the most declarative way is to move state that can affect buttons up to the page level and render the buttons in layout from e.g. move state in RunList up to ExperimentDetails's state, and render buttons directly from ExperimentDetails. https://reactjs.org/docs/lifting-state-up.html

Moving state up might sound daunting at first, but React has also built React hooks, they allow making abstractions on state logic, so that moving some state can be as simple as moving a hook call from child to parent and pass needed props to the child.

[zijianjoy]

I agree with moving the state up to parent so the parent has all the information it needs to determine buttons arrangement. Thank you Yuan!

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[Bobgy]

/lifecycle frozen