Skip to content
This repository was archived by the owner on Jun 17, 2025. It is now read-only.

Conversation

@JLou
Copy link
Contributor

@JLou JLou commented Dec 14, 2022

Related issue

Description of the issue

So, even with an esm build, the bundlers were not able to tree-shake the all package. This is due most likely to HOC (higher order components). The problem is described here on the webpack documentation.

I setup a small vite project to reproduce it.

This is the app :

import { useState } from "react";
import reactLogo from "./assets/react.svg";
import "./App.css";
import "@axa-fr/react-toolkit-all/dist/style/af-toolkit-core.css";
import { Button, Action } from "@axa-fr/react-toolkit-all";

function App() {
  const [count, setCount] = useState(0);

  return (
    <div className="App">
      <div>
        <a href="https://vitejs.dev" target="_blank">
          <img src="/vite.svg" className="logo" alt="Vite logo" />
        </a>
        <a href="https://reactjs.org" target="_blank">
          <img src={reactLogo} className="logo react" alt="React logo" />
        </a>
      </div>
      <h1>Vite + React</h1>
      <div className="card">
        <Button
          id="batman"
          onClick={(e) => {
            console.log(e);
            setCount((c) => c + 1);
          }}
        >
          Count is {count}
        </Button>

        <Action id="mabi" icon="switch" onClick={(e) => console.log(e)} />
        <p>
          Edit <code>src/App.tsx</code> and save to test HMR
        </p>
      </div>
      <p className="read-the-docs">
        Click on the Vite and React logos to learn more
      </p>
    </div>
  );
}

export default App;

Using 2.0.0 this is the bundle you get is way too big for only using Alert and Button.
toolkit makes up for more than 82% of the final bundle

Adding the flag "sideEffect": false to the package.json, we however get a much smaller bundle:

toolkit makes up for less than 3% of the final bundle

The next step, was to start cleaning up these HOC which brought nothing but pain.
Firstly, the typing was wrong because of how complex all of it was:
Button cannot be used as a JSX component

Secondly, the HOC removed the event from the onclick event to replace it with an simple object containing only the id
logging the event show only the id.

What I did is merge the original synthetic react event with the simple id object to keep existing behaviour, while making sure users would get the expected event in their onclick handler.
id shows up alongside the event properties

I'm opening this PR to start a discussion about what to do for these HOC scattered across the code.

Person(s) for reviewing proposed changes

You can add @mention here

PIERMÉ Jean-Lou added 2 commits December 14, 2022 15:29
Because of all the HOC and other weird stuff going on
in the code, the bundlers are unable to determine what
code branches can be trimmed (�tre  shaking).
Marking the library as side-effect free makes it believe
all functions are pure.
ref: https://webpack.js.org/guides/tree-shaking/#mark-the-file-as-side-effect-free
The HOCs not only disturb the bundler's process to understand which code
does what, it also makes typing a pain.
Moreover, the HOC used here removed the react event and replaced it
with only the id of the HTML element.
This version makes typing slightly simpler, and gives user the complete
event while maintaining existing behaviour.

type ButtonCoreProps = ComponentPropsWithoutRef<typeof ButtonCore>;
export type ButtonProps = WithClickIdProps<ButtonCoreProps, 'onClick'>;
interface MyEvent extends React.MouseEvent<HTMLButtonElement> {
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: renommer l'événement en ButtonEvent ou autre, WithIdButtonEvent , ...

onClick: React.EventHandler<MyEvent>;
};

function useWithEventId<T extends SyntheticEvent>(fn: EventHandler<MyEvent>) {

Choose a reason for hiding this comment

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

Pourquoi le nommer comme un hook React alors que ce n'est pas un hook, c'est trompeur :(

@JLou JLou added this to the V3.0.0 milestone Sep 5, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants