Surface code review

[dmtrKovalenko]

Here are some of my foundings on the current material-ui-x codebase.

Do not take it personally, just some of my observations

1. Not using clsx module to combine class names

Such expressions are so unreadable (https://github.com/mui-org/material-ui-x/blob/master/packages/grid/src/components/cell.tsx#L6):

let cssClasses = `${CELL_CSS_CLASS} ${cssClass || ""} ${
  showRightBorder ? "with-border" : ""
}`;

cssClasses += !align || align === "left" ? "" : ` ${align}`;

2. Classname expressions className={'country-flag'} are a little bit weird.
   Which is potentially caused by 1.
3. The function component should have return all the time because any potential change that will require variable or state hook in the component will require to change the whole component code. Which harms git history.

export const IsDone: React.FC<{ value: boolean }> = React.memo(({ value }) =>
  value ? <DoneIcon fontSize={"small"} /> : <ClearIcon fontSize={"small"} />
);

export const IsDone: React.FC<{
  value: boolean;
}> = React.memo(({ value }) => {
  return value ? (
    <DoneIcon fontSize={"small"} />
  ) : (
    <ClearIcon fontSize={"small"} />
  );
});

4. Do we really need to wrap all the components in React.memo? Memoization costs runtime performance. And probably we do not need to make premature optimizations:

MOST OF THE TIME YOU SHOULD NOT BOTHER OPTIMIZING UNNECESSARY RERENDERS. React is VERY fast and there are so many things I can think of for you to do with your time that would be better than optimizing things like this. In fact, the need to optimize stuff with what I'm about to show you is so rare that I've literally never needed to do it in the 3 years I worked on PayPal products and the even longer time that I've been working with React.
cc https://kentcdodds.com/blog/usememo-and-usecallback

5. Avoid using inline styles style={{ display: 'flex', justifyContent: 'space-between' }} (https://github.com/mui-org/material-ui-x/blob/master/packages/grid-data-generator/src/renderer/incoterm-renderer.tsx, https://github.com/mui-org/material-ui-x/blob/master/packages/grid-data-generator/src/renderer/progress-bar.tsx and other)

6. Grid cell field prop must be well-typed string union which will autocomplete the other available props (https://github.com/mui-org/material-ui-x/blob/master/packages/grid/src/components/cell.tsx#L6)

7. Make sure that we are using strict mode in tsconfig.json. I am seeing some functions without argument types, that would be inferred as any

8. Expressions like icons: { [key: string]: React.ReactElement }; are so unsafe. key must be well-typed for autocomplete and type system experience
   (https://github.com/mui-org/material-ui-x/blob/master/packages/grid/src/components/column-headers.tsx#L13)

9. Probably we need some convention about the body of the functional component. IMHO it is much more readable when all the state/ref hooks are going first, then declaring effects, then callbacks and than return. It is similar to what we have with class components (1. state in the constructor, 2. methods, and the last render method) E.g. this component is really hard to read.

10. Avoid using 1-letter variable names, even for maps {columns.map((c, idx) => ( (https://github.com/mui-org/material-ui-x/blob/master/packages/grid/src/components/column-headers.tsx#L94)

11. This .map loop is to long and too nested. Really hard to read, probably need to split in some reusable functions to improve readability.

12. Do we really need constants for css classes? Looks like it is too different from material-ui styling solution (https://github.com/mui-org/material-ui-x/blob/master/packages/grid/src/constants/cssClassesConstants.ts)

13. Events must be properly typed MouseEvent is native dome event. React.MouseEvent is react synthetic event and must be used as React.MouseEvent<HTMLElement> to prevent explicit casts later on and improve typings. (https://github.com/mui-org/material-ui-x/blob/master/packages/grid/src/hooks/root/useApi.ts#L58

Overall typescript doesn't work well right from what I am seeing. For example probably we want to use import * as React from 'react' in this project as well to get better access to the react typings.

Probably we don't need to use explicit type definitions everywhere like here:

  const getColumnFromField: (field: string) => ColDef = field => stateRef.current.lookup[field];
  const getAllColumns: () => Columns = () => stateRef.current.all;
  const getColumnsMeta: () => ColumnsMeta = () => stateRef.current.meta;
  const getVisibleColumns: () => Columns = () => stateRef.current.visible;

We could make the return value to be inferred automatically. The same for anonym objects.

It is used ! operator really rarely which means that we have an unsound type system and we are going to crash here 😱 just like here. Probably we want to use ? operator?

14. Unnecessary spaces between imports. Used a lot, example from https://github.com/mui-org/material-ui-x/blob/master/packages/grid/src/hooks/root/useKeyboard.ts

import { useEffect } from 'react';

import { useLogger } from '../utils/useLogger';
import { KEYDOWN_EVENT, KEYUP_EVENT, MULTIPLE_KEY_PRESS_CHANGED } from '../../constants/eventsConstants';

import { GridApiRef } from '../../grid';

15. Do we really need to use forceUpdate's in each hook? Probably we can avoid using it?

16. Do we need to duplicate ColumnsContainer.displayName = 'ColumnsContainer'; for each component? It will be inferred by react automatically

17. Is it intended that we are using global classnames? I suppose we need to remove usage of the global class names like here. It is potentially dangerous because of accidental overrides. But I suppose it will be redone.

18. Probably we need to use prettier for code formatting, to increase readability for other contributors.

[oliviertassinari]

• 4 - If there are components that use React.memo but accepts a React element too, we should remove it. It's will continue to be de-optimized if the React element isn't memorized too. Here is a good example.

https://github.com/mui-org/material-ui-x/blob/da7e86d38fd895ba71f6fa00cafe40d2e6ff0969/packages/grid/src/components/sticky-container.tsx#L4

The children prop is never identical between two renders, we systematically waste CPU cycles in the React.memo().

• 5 - I guess the only exception we should consider it for styles like top and left that can change between each frame? It makes me think of Avoid inline style as much as possible material-ui-pickers#1679 :)
• 9 - 💯 This resonates with the discussion we have started in Improve onError validation  material-ui-pickers#1730 (comment). While we have a standard for this on the main repository for JavaScript sources. We don't have for TypeScript sources.
  So we probably need to keep refining how we want to write components. I think that the main aspect here is that we get a standard so we can top arguing about how it should be done. At the end of the day, I think that once we are used to one arbitrary coding style,
  it will no longer make a difference 😁.
• 10 - 💯
• 11 - I guess it's a coding preference? Personally, I prefer 1,000 LOCs file with long methods than having to browse 10 files of 100 LOCs and small methods, but it's only because I'm used to working like this. As you guys see fit.
• 12 - We plan to push further in this direction in v5. I think that it will help with unstyled components.
• 16 - 💯 to be removed :)
• 17 - The original intent was for performance. I guess we would need to benchmarked to see if really necessary, I suspect it was and is.
• 18 - I have seen a configuration file, so I guess it's already used? In Early feedback #1, I mention using the exact same prettier configuration as the main repository.

[dmtrKovalenko]

Regarding 9th point. I meant that this code block

let cssClassProp = { cssClass: ‘’ };
    if (c.cellClass) {
      if (typeof c.cellClass === ‘string’) {
        cssClassProp = { cssClass: c.cellClass };
      } else if (Array.isArray(c.cellClass)) {
        cssClassProp = { cssClass: c.cellClass.join(' ’) };
      } else if (typeof c.cellClass === ‘function’) {
        const params: CellClassParams = getCellParams(row, c, rowIndex, value, api!.current!);
        cssClassProp = { cssClass: c.cellClass(params) as string };
      }
    }

could be transformed into the reusable function call to improve readability and self-documenting.

const cssClassProp = parseColumnCssClass(c.cellClass)

This like one of the most basic principles of functional programming (which I am a fan of, so you could ignore my reviews XD). In functional programming, we are writing not what our code should do but how it works.

For instance, in this example, I need to read and think some time to understand that this code block is actually parsing class name because it can be array or function or string.

[oliviertassinari]

@dmtrKovalenko I should have consolidated your feedback into new issues and #6. If you see something missing, please open a new issue per item, thanks for giving a 👁️ :).