-
-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
feature: introduce Feature Composing System for reducing bundle size #6368
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
7115260
to
ccebeb6
Compare
function __motion<T extends ChakraComponent<any, any>>( | ||
el: T, | ||
): CustomDomComponent<PropsOf<T>> { | ||
const m = motion as any | ||
if ("custom" in m && typeof m.custom === "function") { | ||
return m.custom(el) | ||
function useMotionSvg() { | ||
const framerMotion = useFramerMotion() | ||
|
||
if (!framerMotion) { | ||
throw new Error( | ||
"You need to add animationFeature to use Checkbox component", | ||
) | ||
} | ||
return m(el) | ||
|
||
return framerMotion.motion(chakra.svg) | ||
} | ||
|
||
// @future: only call `motion(chakra.svg)` when we drop framer-motion v3 support | ||
const MotionSvg = __motion(chakra.svg) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
framer-motion v3 support is already dropped
if (!framerMotion) { | ||
throw new Error( | ||
"You need to add animationFeature to use Checkbox component", | ||
) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can fallback to the component without animation
ccebeb6
to
7d9c6c3
Compare
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 7d9c6c3:
|
@@ -45,11 +45,13 @@ | |||
"@zag-js/focus-visible": "0.1.0" | |||
}, | |||
"devDependencies": { | |||
"@chakra-ui/react": "2.2.4", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this is not good. Will be fixed after deciding package structure.
export function createToastFeature( | ||
toastOptions?: ToastProviderProps, | ||
): ChakraFeature { | ||
return { | ||
id: "toast", | ||
Provider(props) { | ||
return <ToastProvider {...props} {...toastOptions} /> | ||
}, | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an independent function so that the reference to ToastProvider
can be dropped by tree-shaking.
@segunadebayo Hi, what do you mean by draft? Should I do something additional to get reviewed or just wait? |
Hi @yukukotani. Based on your comment, "This is just a rough PoC". There are a lot of breaking changes this will introduce and I want to give you the chance to prove your ideas before reviewing them. |
@segunadebayo Thanks, understood. I think we can introduce this without any breaking changes but anyway the PR is ready for getting feedback about my idea! |
Just a general feedback, I think it's a step in the good direction, but it won't solve the root problem. Framer-Motion is a big 30 KB gzipped monolith. Given what Chakra-UI uses it for, it could be replaced by a Having your component breaks without implementing animation in the provider might not be the best option. Anything that is not required on load should be lazy-loaded. I do not need an animation on checkboxes when my page load. I might need one on hover, focus or click, but that's what dynamic imports are for. The goal is to avoid loading a gigantic amount of JS on first load, which Chakra is crippled by at the moment. You can't tree-shake part of Framer-Motion, which means they would dynamically import 30 KB gzipped of code when lazy-loaded. There are only 3 solutions to this problem:
The solution Both solutions Your solution could be a intermediary step for the time being, but as I said, it won't solve the root cause of the problem. A 30KB gzipped monolith is simply too big of a liability and it will always affect performance no matter how or when you load it. |
@TheThirdRace Thank you for the feedback! First, I totally agree with your opinion that Framer-Motion is the root problem and we should remove that. But we will use react-transition-group or something like that to replace Framer-Motion and it's redundant for users who don't need animations. They don't want to load animation-related codes even if it's lazy-loaded. So Feature Composing System will still be effective for reducing bundle size after Framer-Motion is wholly removed. Also, global components such as Toast are bundled regardless of whether it's used or not. Feature Composing System can resolve that.
Yes. We can fall back to the non-animated component if animation is not implemented in the provider. So the summary of my opinion is:
|
Yes, I agree with the conclusions above. Removing framer-motion is a priority for me in the next major release. I believe we can design the components to fit into any animation library or native CSS animations, and I have ideas around this. Thanks for taking the time to work on this @yukukotani. |
@TheThirdRace Framer Motion does allow lazy loading animations https://www.framer.com/docs/guide-reduce-bundle-size/#async-loading So I can better understand the problem (as reducing bundle size is something I'm actively thinking about) - what is it about the above approach that falls short? |
It's not as much the lazy-loading per se than the actual bundle size. The way Chakra-UI uses Framer-Motion, the package adds By itself, this is already way too big. For comparison sake, in a typical app, There have been attempts to use lazy-loading in Chakra-UI in the past. I know of at least 1 PR I followed back in the days that did everything possible to reduce Framer-Motion bundle size and came up at It's very hard to justify a 25 to 32 KB gzipped dependency when you know you can replace most animations with 2 css classes and a prop. I want to point out I'm not denying the hard work made on Framer-Motion. It's actually an awesome library and if I needed an animation framework , it would definitely be at the top of the list. It's just that in the context of Chakra-UI, it's not a good design decision for them to decide for us what's gonna be our animation framework for our whole app. Their use of animations is very simple and minimal, there's no point in using a nuclear strike to squash a fly 😅 |
Related to #4975, #3696
📝 Description
This PR is a proposal for the Feature Composing System for reducing bundle size. This is just a rough PoC implementation to get reviews about the approach.
The concept is like dependency injection. Features directly depend on external large libraries such as framer-motion. Child components refer to it via Context so that they don't need to have large dependencies directly.
Current
ChakraProvider
is still fully featured, so users can start with it, then optimize by replacing it withChakraComposingProvider
on they need it.⛳️ Current behavior (updates)
Case 1:
With the code below, there is no animation but
framer-motion
will be included into bundle, becauseChakraProvider
containsToastProvider
that depends onframer-motion
.Case 2:
With the code below,
framer-motion
will be included into bundle, becauseCheckBox
depends onframer-motion
.🚀 New behavior
Case 1
With the code below,
framer-motion
will be included into bundle, becausecreateToastFeature
containsToastProvider
that depends onframer-motion
.Once you removed
toastFeature
,framer-motion
will be removed by tree-shaking anduseToast
will stop working.Case 2
With the code below,
framer-motion
will be included into bundle, becausecreateAnimationFeature
depends onframer-motion
.Once you removed
animationFeature
,framer-motion
will be removed by tree-shaking butCheckbox
will stop working.💣 Is this a breaking change (Yes/No):
No
📝 Additional Information
Remaining points