aidenybai / million

Optimize React performance and make your React 70% faster in minutes, not months.
https://million.dev
MIT License
16.33k stars 575 forks source link

Name Of Function Is Not Descriptive #339

Closed tobySolutions closed 1 year ago

tobySolutions commented 1 year ago

For CodeDay Labs students only, do not assign to anyone else

The function h in /million/packages/jsx-runtime/index.ts has the following code:

import type { Props, VNode } from '../million';

export const h = (
  type: string,
  props: Props | null = {},
  ...children: VNode[]
): VNode => {
  if (props === null) props = {};
  props.children = children;
  return {
    type,
    props,
  };
};
export { h as createElement, h as jsx, h as jsxs };

The above code is problematic and gives a bad developer experience because of the bad naming of h that it has.

In the above block of code, we can see that we have a Typescript function that is meant to return a Virtual DOM node based on the parameters: type (type of dom element e.g p, h1, and so on), props (like attributes of a React dom component which children is technically a part of) and children (in this case the content of the component and what is being with dom element).

The code above just returns something of the following format to us h('div', { id: 'foo' }, 'hello') which is actually <div id="foo">hello</div>

What we need to do are the following steps:

  1. Pick a better name (e.g element, domElement or virtualDomElement)
  2. Edit the function name to whatever you've decided earlier, assuming I chose element, I would have:
    
    import type { Props, VNode } from '../million';

export const element = ( type: string, props: Props | null = {}, ...children: VNode[] ): VNode => { if (props === null) props = {}; props.children = children; return { type, props, }; };

4. In the last line where you export (`export { h as createElement, h as jsx, h as jsxs };`), change the function name there too, because failure to do this will break the codebase. You should then have something like this:

```jsx
import type { Props, VNode } from '../million';

export const element = (
  type: string,
  props: Props | null = {},
  ...children: VNode[]
): VNode => {
  if (props === null) props = {};
  props.children = children;
  return {
    type,
    props,
  };
};

export { element as createElement, element as jsx, element as jsxs };
SukkaW commented 1 year ago

IMHO we shouldn't change the name h as h is widely used in many virtual dom or related libraries. Renaming it would cause more chaos.

Preact

https://preactjs.com/guide/v10/api-reference#h--createelement

import { h } from 'preact';

Vue.js

https://vuejs.org/api/render-function.html#h

import { h } from 'vue'

snabbdom

https://github.com/snabbdom/snabbdom#h

import { h } from 'snabbdom'

virtual-dom

https://github.com/Matt-Esch/virtual-dom

var h = require('virtual-dom/h');

Solid.js

https://www.solidjs.com/examples/simpletodoshyperscript

import h from "solid-js/h";

hyperscript

https://github.com/hyperhype/hyperscript

var h = require('hyperscript')

vobyjs

https://github.com/vobyjs/voby#h

import {h} from 'voby';
tobySolutions commented 1 year ago

Thank you @SukkaW. @aidenybai rightly pointed that out to me. I'll close this issue now.