Discrepancy with main repository

[oliviertassinari]

The purpose of this issue is to identify misalignment between the main repository and this one, see what improvement we can bring in any direction.

While it's unclear how this package will evolve in the future, staying standalone or merging with the main repository as we plan for the pickers. So far the great reason not to merge is to keep the effort private and allow fast POC iterations. But what about once we make it public? I think that our default answer to the problem would be to go all-in in the mono-repository approach unless there is a great reason not to, something that would outweigh the cost (like we have right now).

The core idea around why this approach can help bring value:

1. Put constraints in place to maximize the consitency of the developer experience on the whole library (user side).
2. Make it easier for an engineer to contribute to the library, as a whole, no matter which component we are used to working on. Experimentations should go through small efforts / RFC and be rollout at 100% when successful, not scattered between components.
3. Share knowledge, tools, and great practices learned the hard way. Any small incremental improvement to the CI, tests, etc compound. It can be very hard to share and enforce different repositories.

List of items

Left is mui-org/material-ui-x, right is mui-org/material-ui. I will make the case for why the right approach might be better at the same time. I would love to hear counter-arguments in the other direction.

• function Foo({ bar, baz }) vs. function Foo(props): Using props as the first variable and destructuring in the body of the function. This allows making the context explicit when reading the signature of the function. We are inside a React component:

  https://github.com/mui-org/material-ui-x/blob/74d4fc9fbfacfc154b10a98a2e99e74596107bc5/packages/grid/_modules_/grid/components/column-header-item.tsx#L21

• classnames utils vs. clsx() dependency. The last discussion we had on the topic can be found in [styles] Replace classnames with clsx material-ui#14152. clsx comes with a babel plugin that rewrites the source to make the function run faster and take less bundle size: https://github.com/merceyz/babel-plugin-optimize-clsx

  https://github.com/mui-org/material-ui-x/blob/fb5af45b5d74ca818fc6945af17400cec0ee21a2/packages/grid/src/components/cell.tsx#L33

• Explicit display name vs. implicit one. The React dev tool should be able to infer the name from anonymous arrow functions and named functions with the .name property. It shouldn't be needed. I believe it's bundle size we can save from the bundle and one less thing to worry about.

  https://github.com/mui-org/material-ui-x/blob/fb5af45b5d74ca818fc6945af17400cec0ee21a2/packages/grid/src/components/cell.tsx#L59

• src/renderer/link-renderer.tsx vs. src/renderer/LinkRenderer.tsx: Naming the filename of the component with the same case of the name of the component make is easier to identify.

  https://github.com/mui-org/material-ui-x/blob/0ad5f5dad131df88df0c77103a22308704a6349b/packages/grid-data-generator/src/renderer/link-renderer.tsx#L12

• [core] Add yarn workspaces #33 npm vs. yarn. It originates from an issue with Lerna. I think that we can have a closer look at the problem once we actually use Lerna to publish code on the main repository, so far, we only use Lerna to execute commands in parallel. If we can get rid of the yarn link requirement, it will be awesome.

• [core] Use same prettier config as main repo #39 120 vs. 100 max code lenght. Prettier recommends against using more than 80. I think that we already did a stretch by going to 100.

  https://github.com/mui-org/material-ui-x/blob/fb5af45b5d74ca818fc6945af17400cec0ee21a2/.prettierrc.js#L2

• [core] Batch small changes #104 import React, { useEffect } from 'react'; vs. import * as React from 'react';. The React bundle isn't tree-shakable, the latter offers a better DX, no need to update the imports when adding or removing the usage of an API.

  https://github.com/mui-org/material-ui-x/blob/fb5af45b5d74ca818fc6945af17400cec0ee21a2/packages/grid/src/hooks/root/useApi.ts#L1

• [core] Use the classnames helper #105 className={'country-flag'} vs. className="country-flag". I believe the convention in the React ecosystem is to us the shorthand version (double quote) anytime it's possible. The React documentation seems to follow this approach, which I would guess is more frequently used in the community.

  https://github.com/mui-org/material-ui-x/blob/fb5af45b5d74ca818fc6945af17400cec0ee21a2/packages/grid-data-generator/src/renderer/country-renderer.tsx#L31

• useCallback((e: any) => vs. useCallback((event: any) => { I think that we should use explicit arguments exclusively. I get that it can be faster to write, but it hard readability, which should have more value overall.

  https://github.com/mui-org/material-ui-x/blob/fb5af45b5d74ca818fc6945af17400cec0ee21a2/packages/grid/src/hooks/virtualization/useVirtualRows.ts#L247

• import syntax. There are a couple of different approaches. Should we accept blank lines in between the imports? Should we sort the imports in a specific order?

  https://github.com/mui-org/material-ui-x/blob/fb5af45b5d74ca818fc6945af17400cec0ee21a2/packages/grid/src/hooks/root/useKeyboard.ts#L3-L5

• ...rest vs. ...other. A pure convention, we always name the other props, other.

  https://github.com/mui-org/material-ui-x/blob/fb5af45b5d74ca818fc6945af17400cec0ee21a2/packages/grid/src/components/column-header-title.tsx#L7

• I will add more items as I identify "gaps" and potential improvements in either direction.

[oliviertassinari]

One year later, we have fixed most of the discrepancies between the core and x repositories. It's the SAME codebase.