elyra-ai / elyra

Elyra extends JupyterLab with an AI centric approach.
https://elyra.readthedocs.io/en/stable/
Apache License 2.0
1.86k stars 344 forks source link

Document best practice for implementing UI components #1594

Open lresende opened 3 years ago

lresende commented 3 years ago

Some of Elyra UI components are very coupled with related business logic and we are discussing refactor them to promote separation of concerns and sort of use MVC code-pattern (see #1405).

This issue will handle the documentation of the best-practice/patterns to adopt during the refactor and creation of new UI components for Elyra.

As for location, I would say either somewhere in Elyra docs or Community repo. Having said that, I believe we could start with Elyra repo and promoting to the community after a few interactions to make it solid first.

bourdakos1 commented 3 years ago

Nick’s guide to a fun frontend

Component Style

All React components should be functional components (no class components). Class components can promote the use of inheritance where React works best with a composition model. For more information see: https://reactjs.org/docs/composition-vs-inheritance.html

interface Props {
    text: string;
}

export const Component: FC<Props> = ({ text }) => {
    return <div>{text}</div>;
}

Hooks

Using functional components will drive the need for the use of hooks. Hooks have two very important rules:

  1. Hooks can only be called in a react functional component or a custom hook
  2. Do not call hook conditionally, inside loops or inside nested functions

The two hooks you will most likely come across are useState and useEffect.

useState

State is used to store a dynamic values produced from a side effect. Examples:

The useState hook is a function that takes a single argument that acts as the initial value of the state. The useState function returns an array with only two values. This array is normally destructured, the first item being the value of the state (which can never be manually modified) and the second item being a setter for that value.

Example:

export const Component: FC = () => {
    const [value, setValue] = useState("Default Value");

    const handleClick = () => {
        setValue("Clicked!")
    }

    return <div onClick={handleClick}>{value}</div>;
}

When calling the setter, it will completely replace the value of the state instead of merging like with this.setState in class components. For this reason it is best to use many smaller states than a large state object.

BAD:

export const Component: FC = () => {
    const [state, setState] = useState({ 
        color: "red", 
        text: undefined,
    });

    const handleColorChange = (color) => {
        setState({
            ...state,
            color,
        });
    }

    const handleTextChange = (value) => {
        setState({
            ...state,
            text: value,
        });
    }

    return (
        <div 
            color={state.color}
            text={state.text}
            onColorChange={handleColorChange}
            onTextChange={handleTextChange}
        />
    );
}

GOOD:

export const Component: FC = () => {
    const [color, setColor] = useState("red");
    const [text, setText] = useState();

    const handleColorChange = (color) => {
        setColor(color);
    }

    const handleTextChange = (value) => {
        setText(value);
    }

    return (
        <div 
            color={color}
            text={text}
            onColorChange={handleColorChange}
            onTextChange={handleTextChange}
        />
    );
}

If you have a collection of state values that need to be updated both simultaneously and independently you might want to reach for the useReducer hook. A good example of this is with data fetching. You probably want a status and data state and when the data is ready, you probably want to update the status in the same action. However, it is normally best to use a library like swr or react-query to handle data fetching so you probably won't come across the useReducer hook very often.

useEffect and Custom Hooks

Effects are were all side effects live. Side effects include data fetching, subscriptions and manual DOM mutations. Since we should be using a library for data fetching and manual DOM mutations are extremely rare the only thing that should show up in effects are subscriptions.

Example:

export const Component: FC = () => {
    useEffect(() => {
        const handleResize = () => {
            // do something
        }

        // subscription to the window resize event.
        window.addEventListener("resize", handleResize);

        // Effects allow you to return a cleanup function. It is important to 
        // always clean up your subscriptions to avoid leaks.
        return () => {
            window.removeEventListener("resize", handleResize);
        }

    // dependency array, an empty array means this will only run on component 
    // mount. If this array has a list of variables, it will rerun when those 
    // variables change.
    }, []);
}

If you find yourself using an effect, it's normally a good idea to package it into a custom hook instead of using it in a component directly. This helps keep concerns separate and logic reusable.

Example:

export const useWindowSize = () => {
    const [size, setSize] = useState({ 
        width: window.innerWidth, 
        height: window.innerHeight,
    });

    useEffect(() => {
        const handleResize = () => {
            setSize({ width: window.innerWidth, height: window.innerHeight });
        }
        window.addEventListener("resize", handleResize);
        return () => window.removeEventListener("resize", handleResize);
    }, []);

    return size;
}

export const Component: FC = () => {
    const { width, height } = useWindowSize();
}

Props and State

React props should never be mutated or used in state. This can cause bugs and stale information. BAD:

export const Component: FC<Props> = (props) => {
    const handleClick = () => {
        props.color = "red";
    }

    return <div onClick={handleClick}>{props.color}</div>;
}

NOT GREAT:

export const Component: FC<Props> = ({ defaultColor }) => {
    // NOTE: `color` state will not update if `defaultColor` prop changes
    const [color, setColor] = useState(defaultColor);

    const handleClick = () => {
        setColor("red");
    }

    return <div onClick={handleClick}>{color}</div>;
}

BEST:

// Uncontrolled component -- see bellow
export const Component: FC<Props> = ({ defaultColor }) => {
    const [color, setColor] = useState();

    const handleClick = () => {
        setColor("red");
    }

    // as long as `color` is undefined the rendered text will be kept updated 
    // with the value of `defaultColor`
    return <div onClick={handleClick}>{color ?? defaultColor}</div>;
}

The above approach is considered an "uncontrolled" component, which means the component fully controls its own state and is "uncontrolled" by it's parent. Most of the time this can be reworked into a "controlled" component where the component's parent handles it's state. If we use a controlled component, in this case, we can remove the need of handling the "default" prop:

export const Parent: FC = () => {
    const [color, setColor] = useState("green");

    const handleChange = () => {
        setColor("red");
    }

    return <Child onChange={handleChange} color={color} />;
}

// Controlled component
export const Child: FC<Props> = ({ color, onChange }) => {
    return <div onClick={onChange}>{color}</div>;
}

A "controlled" component isn't better than an "uncontrolled" component and an "uncontrolled" component isn't better than a "controlled". There will always be an "uncontrolled" component somewhere, because something needs to manage the state. A good rule of thumb is if the parent needs access to the child's state, then you should reach for a "controlled" component.

Utility Functions

  1. A utility function should not be a static method of a class, it should simply be an exported function.

  2. Utility functions should not contain any UI or async actions (data fetching).

  3. The utility functions of in a file should all be strictly related.

BAD:

export class Utils {
    static filter = (list, status) => list.filter(list => list.status === status);
}

BETTER:

export const filter = (list, status) => list.filter(list => list.status === status);

BEST:

// Don't create a file of utility functions composed of extremely basic tasks.

Before creating a utility function ask yourself if there is another way to reduce code reuse without it.

For example, let's say we had a TODO list that needed to be filtered to only show completed items, and a few components needed this filtered list

NOT IDEAL:

// please excuse this extremely contrived example

export const filter = (list, status) => list.filter(list => list.status === status);

export const List: FC = () => {
    const todoList = useTodoList();

    const completed = filter(todoList, "complete");

    return <div>{completed.map(c => <div>{c.title}</div>)}</div>;
}

export const Count: FC = () => {
    const todoList = useTodoList();

    const completed = filter(todoList, "complete");

    return <div>{completed.length}</div>;
}

Assuming these two components don't have a common parent for some reason and we need to refetch the full todo list for both, we could modify useTodoList function to accept a filtering option. BETTER:

export const List: FC = () => {
    const completed = useTodoList({ filter: "complete" }));

    return <div>{completed.map(c => <div>{c.title}</div>)}</div>;
}

export const Count: FC = () => {
    const completed = useTodoList({ filter: "complete" }));

    return <div>{completed.length}</div>;
}

Most of the time components will share a parent, so we can do something like this:

export const Parent: FC = () => {
    const list = useTodoList());

    const completed = list.filter(l => l.status === "complete");

    return (
        <div>
            <Count count={completed.length} />
            <List list={completed} />
        </div>
    );
}

export const List: FC<Props> = ({ list }) => {
    return <div>{list.map(c => <div>{c.title}</div>)}</div>;
}

export const Count: FC<Props> = ({ count }) => {
    return <div>{count}</div>;
}

Layers

An Elyra UI extension has three layers:

  1. The activation layer
    • pure typescript (no react/UI)
    • can import from @jupyterlab
    • sets up commands and creates widgets
  2. The connection layer
    • can use react
    • can import from @jupyterlab
    • binds jupyter widgets to react components
    • should be as thin as possible (think velcro not superglue)
  3. The UI layer
    • can use react
    • CANNOT import from @jupyterlab
    • should be 100% useable from outside of jupyter with no code changes

Business Logic

Most business logic should be accessable to the connection layer only. Elyra's UI extensions shouldn't contain much business logic asside from connecting the fontend to the backend.

bourdakos1 commented 3 years ago

[WIP] Example refactor of pipeline-editor-extension

I would start with PipelineService.tsx

I would find any GET requests and try to recreate the functionality using swr.

example:

// Change this:
export class PipelineService {
    static async getRuntimeImages(): Promise<any> {
        try {
            // we should have this typed.
            let runtimeImages = await MetadataService.getMetadata('runtime-images');

            // sort mutates so we can change `x = x.sort` to `x.sort`
            // This should also probably be moved to the server
            runtimeImages = runtimeImages.sort(
                (a: any, b: any) => 0 - (a.name > b.name ? -1 : 1)
            );

            // we shouldn't throw any errors
            if (Object.keys(runtimeImages).length === 0) {
                return RequestErrors.noMetadataError('runtime image');
            }

            // If we want our data to be sorted we shouldn't create a map. if order
            // matters we should use a list.
            // the only places that use this map, just convert it back to an array
            // so we should drop this.
            const images: IDictionary<string> = {};
            for (const image in runtimeImages) {
                const imageName: string =
                runtimeImages[image]['metadata']['image_name'];
                images[imageName] = runtimeImages[image]['display_name'];
            }
            return images;
        } catch (error) {
            // This is redundant
            Promise.reject(error);
        }
    }
}

// To this:
interface RuntimeImage {
    name: string;
    display_name: string;
    metadata: {
        image_name: string;
    }
}

export const useRuntimeImages = () => {
    // We will have a global config that handles the base url and the actual fetching.
    const { data, error } = useSWR<RuntimeImage[]>('/metadata/runtime-images');

    data.sort((a, b) => a.name > b.name ? 1 : -1);

    return { data, error };
}

// or this if we do sorting on the server:
export const useRuntimeImages = () => {
    return useSWR<RuntimeImage[]>('/metadata/runtime-images');
}
lresende commented 3 years ago

Let's make this a PR to https://github.com/elyra-ai/elyra/tree/master/docs/source/developer_guide

bourdakos1 commented 3 years ago

I did a quick look through ui-components since they shouldn't contain any complicated logic and tried list the hooks that would need to be used if converted to a function component. If people want to start learning the basics of hooks this should be a good place to start:

Hook free

Basic

Intermediate