Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Creating light components #259

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Creating light components #259

wants to merge 1 commit into from

Conversation

Nefcanto
Copy link

@Nefcanto Nefcanto commented Nov 6, 2024

The proposal for light components. Light components are even lighter than function components. They don't accept props. They don't have states. They are just a simple JSX constant/variable that can be rendered like a normal component using <LightComponent />.

The proposal for light components
Comment on lines +15 to +22
```
const Contacts = <div>
<div>some phone number</div>
<div>some email</div>
</div>

<Contacts />
```
Copy link

@slorber slorber Nov 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just do this instead?

const contacts = (
  <div>
   <div>some phone number</div>
   <div>some email</div>
  </div>
)


const app = (
  <>
    <h1>My App</h1>
    {contacts}
  </>
);

I really don't see the point of introducing this new concept. Instead of light components, you can simply use constant JSX elements.

Comment on lines +118 to +128
```
const Contacts = <div>Some contact data here</div>

const JsxRenderer = ({ jsx }) => {
return <div>
{jsx}
</div>
}

<JsxRenderer jsx={Contacts} />
```
Copy link

@slorber slorber Nov 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the point of JsxRenderer here?

You can just write this and skip this useless boilerplate

const Contacts = <div>Some contact data here</div>

{Contacts}

Copy link

@slorber slorber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Light components are even lighter than function components. They don't accept props. They don't have states. They are just a simple JSX constant/variable

So, they can just be plain constant JSX elements

that can be rendered like a normal component using <LightComponent />.

It's unclear what's the point of allowing this instead of just {LightComponent}. If it's only about removing boilerplate, {LightComponent} saves 2 extra chars 😄.

@Nefcanto
Copy link
Author

Nefcanto commented Nov 6, 2024

@slorber, assume you write this code:

import { SetPrice } from "pricing"
import { Images } from "media"
import { Categories } from "taxonomy"

const entityActions = <>
    <SetPrice />
    <Images />
    <Categories />
</>

The point is, how do you know which is a simple JSX and which is a function component?

The above code would throw an exception if:

const SetPrice = <EntityAction
    title="Set price"
    dialog={PriceDialog}
/>

export default SetPrice

and

const Images = () => {

    const { entity } = useContext(EntityContext)

    return <EntityAction
        title="Manage images"
        link={`/media/images?entity=${entity.id}`}
    />
}

export default Images

and

class Categories extends React.Component {
  render() {
    return <EntityAction
        title="Manage categories"
        link={`/category/manage?entity=${entity.id}`}
    />
  }
}

export default Categories

See the difference?

A developer does not need to look into the source of a third-party library (or libraries from other team members) to change its syntax for function components or class components.

However, in the case between a JSX and a component, the rendering syntax is different. This means that the developer should know the internals of another library to change the rendering syntax.

It even gets worse for refactoring. When a component needs to become more complex and changes from a simple JSX into a function component, then you should see failure across any place that has used it as {SomeCompnent} and manually change them all to <SomeComponent />.

@Nefcanto
Copy link
Author

Nefcanto commented Nov 6, 2024

It's unclear what's the point of allowing this instead of just {LightComponent}

@slorber two points:

  • No need to know the internals of exports of a third-party library (or a team-mate's module) to change the rendering syntax
  • No need to change the rendering syntax in case of a refactor

@slorber
Copy link

slorber commented Nov 6, 2024

I understand the consistency usage, but still think it's a feature for the developer to have to understand what is constant and what is dynamic on the call site.

how do you know which is a simple JSX and which is a function component?

TypeScript?

CleanShot 2024-11-06 at 16 21 05

It even gets worse for refactoring. When a component needs to become more complex and changes from a simple JSX into a function component, then you should see failure across any place that has used it as {SomeCompnent} and manually change them all to .

If a JSX constant becomes a React component, I actually want to know about it and refactor all the call sites that assumed it was constant.

If that refactor happens, there are probably props to add to that component anyway, otherwise why would you transition them from constant element to component? The only case I can see is if all the props you add are optional, but again I'll probably want to double-check all the call sites.

@josephsavona
Copy link
Contributor

josephsavona commented Nov 6, 2024

Thanks for the proposal! I don't think we're likely to proceed with this RFC. As @slorber already noted, this use-case can be achieved by assigning JSX to a constant. Or, just defining a function component that returns static JSX. React Compiler will already optimize such a component so that the JSX content is created once per instance, and we will likely explore an optimization that hoists the static JSX out of the component to make it equivalent to

const element = <div>...</div>
function Component() {
  return element;
}

In other words, the new concept here isn't actually unlocking any new capabilities/performance, so it's strictly cognitive overhead.

@Nefcanto
Copy link
Author

Nefcanto commented Nov 7, 2024

If that refactor happens, there are probably props to add to that component anyway, otherwise why would you transition them from constant element to component?

@slorber, Contexts. The constant is changed into a component, but the dynamic props are taken from a higher context.

This code (taken from our real codebase):

import AdminPanelSettingsIcon from "@mui/icons-material/AdminPanelSettings"
import { EntityAction } from "List"
import RolesDialog from "./Dialog"

const ManageRoles = <EntityAction
    title="AccountsManageRoles"
    icon={AdminPanelSettingsIcon}
    dialog={RolesDialog}
/>

export default ManageRoles

Becomes this code:

import { useContext } from "react"
import AdminPanelSettingsIcon from "@mui/icons-material/AdminPanelSettings"
import { EntityContext } from "Contexts"
import { EntityAction } from "List"
import RolesDialog from "./Dialog"

const ManageRoles = () => {

    const { entity } = useContext(EntityContext)

    return <EntityAction
        title="AccountsManageRoles"
        icon={AdminPanelSettingsIcon}
        goTo={`/roles?entityType=${entity?.relatedItems?.entityType}&entityGuid=${entity?.guid}&pet=${entity?.relatedItems?.entityType}&pid=${entity.id}`}
    />
}

export default ManageRoles

As you can see, in both cases we can call it without props. <ManageRoles />.

And with all due respect, in a refactor the ultimate skill is to change the internals without changing the externals. Change in externals is not refactoring, it's redefining contracts.

A simple search related to refactoring shows that. We can find it in Wikipedia too:

code refactoring is the process of restructuring existing source code—changing the factoring—without changing its external behavior.

@Nefcanto
Copy link
Author

Nefcanto commented Nov 7, 2024

@slorber, thank you for mentioning the TypeScript. I see that it indeed helps to find out the problem while developing. Yet for a lot of reasons many teams don't use TypeScript.

@Nefcanto
Copy link
Author

Nefcanto commented Nov 7, 2024

@josephsavona, thank you for responding. Facebook created React and as once claimed in the original legacy docs:

At Facebook, we use React in thousands of components, and we haven’t found any use cases where we would recommend creating component inheritance hierarchies.

This was a very unique way of claiming that we know it better, OK?. And it was true. You guys knew it better. To the extent that many people including me left Angular altogether and migrated to React.

Yet the point is that while you know it better in general, you don't know everything.

Thousands of projects are built upon React, and none has come to our request. Why? Because they are not this dynamic. If you consider my other proposal (React reflection) you realize that our use cases are here because we go to extremes.

You see, we have built an SPL. This means that we create thousands of applications using this meta-framework (built upon other frameworks like yours). While it's not fair to Next.js, to understand the point you can compare us to it for example.

As an example. consider this form (real code) in `/Blog/Post/Form.jsx:

import {
    DialogForm,
    LongText,
    Slug,
    Title,
} from "Form"
import AuthorField from "../Author/Field"

const inputs = <>
    <Title />
    <Slug />
    <LongText
        placeholder="InfraSummary"
        property="Summary"
    />
    <AuthorField />
</>

const PostForm = <DialogForm
    entityType="BlogPost"
    help="PostForm"
    humanReadableEntityType="BlogPost"
    inputs={inputs}
/>

export default PostForm

These are all injected dynamically into this form (and hundreds of other forms):

1- Action buttons (save + cancel)
2- Title
3- i18n & l10n
4- Validation (save button is not active until form becomes valid)
5- UX (no layout shift, the first field gets focused when the dialog opens, prevents and warns about unsaved data on navigation, detecting when the form becomes dirty )
6- CRUD and API communication (including form mode for creation and edition)
7- Managing each field (initial value, current value, is dirty, is changed, is valid, ...)

We have seen a lot of other solutions built upon React. None comes close to this level in terms of being dynamic. Therefore we encounter problems that others have not encountered.

You have thousands of components, and we also have thousands of them (2273 at this moment). So we do suffer when we change a constant JSX into a component (to make it internally dynamic) and we have to change hundreds of other components manually. This is a true pain. It doesn't show itself for a simple web app with at most 100 components. Especially when they are all loaded in the same VS Code instance. But our components are spread across more than 100 modules each in a different Git repository. We're even more extended than Next.js because it's just a meta-framework, while we are a meta-framework + a lot of pre-built modules like Blog, Products, Orders, Shipment, Settings, Dashboards, Reporting, Units, Social, Tasks, etc.

Sometimes a change does not take that much on your side. If this change does not take that much, please do us a favor and consider it. If it takes much time, then drop it.

@guillaumebrunerie
Copy link

guillaumebrunerie commented Nov 7, 2024

Here is a suggestion for you: write a custom Babel plugin that automatically adds () => to all top level variables containing only JSX and whose name starts with a capital letter (turning JSX constants into function components). This way you will get your desired API without having to change React in any way, and you get more control over it if needed in the future.

Assuming your build process already uses Babel, I found it actually surprisingly easy to write custom babel plugins, here is an example wrapping all function components with observer (from MobX), as I found it too annoying to do it by hand: https://github.com/guillaumebrunerie/LD56/blob/main/tools/babel-plugin-auto-observe.js

@Nefcanto
Copy link
Author

Nefcanto commented Nov 7, 2024

@guillaumebrunerie, thank you for the suggestion. We use Vite. Thus I think we should try creating plugins for esbuild. However, two points come to my mind that may not make this solution successful:

  1. When we get errors, the error messages do not match the original code (because we have changed the original code on the fly before giving it to the compiler)
  2. It's not just top-level JSX variables/constants. They sometimes go deeper. This becomes very tedious to find out all instances of them.

@guillaumebrunerie
Copy link

@guillaumebrunerie, thank you for the suggestion. We use Vite. Thus I think we should try creating plugins for esbuild.

The project I shared above uses Vite as well and it wasn't a problem.

However, two points come to my mind that may not make this solution successful:

  1. When we get errors, the error messages do not match the original code (because we have changed the original code on the fly before giving it to the compiler)

Babel plugins automatically generate source maps, so you should see your original code (at least it seems to be so for me).

  1. It's not just top-level JSX variables/constants. They sometimes go deeper. This becomes very tedious to find out all instances of them.

It's actually the opposite, by default Babel traverses the whole AST, so you automatically get all variables in nested scopes as well (whether you actually want to define nested function components is another question).

@Nefcanto
Copy link
Author

Nefcanto commented Nov 7, 2024

@guillaumebrunerie, thank you. I give it a try.

@EECOLOR
Copy link

EECOLOR commented Nov 15, 2024

@Nefcanto Couldn't you use a custom jsx function?

https://esbuild.github.io/content-types/#using-jsx-without-react

In this function you could detect the first argument, if it is not a string or function you can wrap it in a function.

@Nefcanto
Copy link
Author

@EECOLOR, I read the link you sent, but it clearly says that those functions are good for when we use JSX outside React. Maybe by changing them other problems get into the scene.

@EECOLOR
Copy link

EECOLOR commented Nov 18, 2024

@Nefcanto I don't see how there are problems replacing those functions with wrappers. Unless you implement them wrong, but you can create tests if you want to be more confident.

This is generic code of a wrapper:

function wrapper(...args) {
  // customization here
  return original(...args)
}

If you want more performance and better readability you specify the arguments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants