This repository was archived by the owner on Jun 17, 2025. It is now read-only.
Remove poor hoc from button and alert#1000
Draft
JLou wants to merge 2 commits intoAxaFrance:masterfrom
Draft
Conversation
8ca20de to
32aa06c
Compare
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.
32aa06c to
b099b08
Compare
SamuelHsn
suggested changes
Jan 9, 2023
|
|
||
| type ButtonCoreProps = ComponentPropsWithoutRef<typeof ButtonCore>; | ||
| export type ButtonProps = WithClickIdProps<ButtonCoreProps, 'onClick'>; | ||
| interface MyEvent extends React.MouseEvent<HTMLButtonElement> { |
Contributor
There was a problem hiding this comment.
suggestion: renommer l'événement en ButtonEvent ou autre, WithIdButtonEvent , ...
johankorger
reviewed
Jan 30, 2023
| onClick: React.EventHandler<MyEvent>; | ||
| }; | ||
|
|
||
| function useWithEventId<T extends SyntheticEvent>(fn: EventHandler<MyEvent>) { |
There was a problem hiding this comment.
Pourquoi le nommer comme un hook React alors que ce n'est pas un hook, c'est trompeur :(
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related issue
Description of the issue
So, even with an esm build, the bundlers were not able to tree-shake the
allpackage. 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 :
Using 2.0.0 this is the bundle you get is way too big for only using Alert and Button.

Adding the flag
"sideEffect": falseto the package.json, we however get a much smaller 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:
Secondly, the HOC removed the event from the onclick event to replace it with an simple object containing 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.

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