fix: tree-shaking in blade components #1045

saurabhdaware · 2023-03-10T04:35:20Z

Description

Reduces bundle-size from 44kb to 13kb (for an App that is only using Button component)

Fixes #959

Before

After

Issue

😇 This creates no side-effects

const MyComponent = () => {};
MyComponent.displayName = 'MyComponent';
export { MyComponent };

😇 This creates no side-effects

const _MyComponent = () => {};
const MyComponent = /*#__PURE__*/ React.forwardRef(_MyComponent); // In this case, PURE comment is added by babel so we don't have to explicitly add it
// MyComponent.displayName = 'MyComponent';
export { MyComponent };

👿 This CREATES side-effects

Here, it considers the .displayName assignment on ref as a side-effect and bundles the entire file

const _MyComponent = () => {};
const MyComponent = /*#__PURE__*/ React.forwardRef(_MyComponent)
MyComponent.displayName = 'MyComponent';
export { MyComponent };

😇 Solution - This creates no side-effects

const _MyComponent = () => {};
const MyComponentWithRef = /*#__PURE__*/ React.forwardRef(_MyComponent)
const MyComponent = /*#__PURE__*/ Object.assign(MyComponentWithRef, {
  componentId: 'MyComponent',
});;
export { MyComponent };

Also, in ActionList's case, having multiple components using each other in same file and defining .componentId was creating side-effects as well. Doing the same Object.assign with PURE comment fixed that as well.

changeset-bot · 2023-03-10T04:35:23Z

🦋 Changeset detected

Latest commit: 4cda6f6

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@razorpay/blade	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

codesandbox-ci · 2023-03-10T04:37:18Z

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 4cda6f6:

Sandbox	Source
razorpay/blade: basic	Configuration

anuraghazra · 2023-03-10T08:15:46Z

packages/blade/src/components/ActionList/ActionListItem.tsx

@@ -130,9 +129,9 @@ const ActionListSection: WithComponentId<ActionListSectionProps> = ({
  );
 };

-ActionListSection.componentId = componentIds.ActionListSection;
+/*#__PURE__*/ Object.assign(ActionListSection, { componentId: componentIds.ActionListSection });


😅 this looks scary

lol check it out now. It got scarier 😆

github-actions · 2023-03-10T09:29:09Z

✅ PR title follows Conventional Commits specification.

divyanshu013 · 2023-03-10T10:23:49Z

packages/blade/src/components/Button/BaseButton/BaseButton.tsx

@@ -443,7 +443,8 @@ const _BaseButton: React.ForwardRefRenderFunction<BladeElementRef, BaseButtonPro
  );
 };

-const BaseButton = React.forwardRef(_BaseButton);
-BaseButton.displayName = 'BaseButton';
+const BaseButton = /*#__PURE__*/ Object.assign(React.forwardRef(_BaseButton), {


We can also add the explanation as comment to one particular component (probably button)

We might need a lint rule to enforce this, easy to miss out. How did you even figure this out lol 😸

Btw is there a babel plugin that automatically does this transformation? Seems like a common issue libraries might be running into 🤔

There is this babel plugin - https://github.com/mbrowne/babel-plugin-pure-static-props. I tried this first but it didn't work. I think it works when props are on react component and not when they are on refs. Even for react components it didn't work at lots of places for some reason. Couldn't find any other plugin either.

We might need a lint rule to enforce this, easy to miss out.

Yes definitely! Exploring if it can be eslint rule or babel plugin.

How did you even figure this out lol 😸

Lol it was tricky

anuraghazra · 2023-03-10T10:45:03Z

packages/blade/src/components/Button/BaseButton/StyledBaseButton.native.tsx

@@ -101,7 +101,8 @@ const _StyledBaseButton: React.ForwardRefRenderFunction<BladeElementRef, StyledB
  );
 };

-const StyledBaseButton = React.forwardRef(_StyledBaseButton);
-StyledBaseButton.displayName = 'StyledBaseButton';
+const StyledBaseButton = /*#__PURE__*/ Object.assign(React.forwardRef(_StyledBaseButton), {


So here, what is the main factor?

The PURE comment or the Object.assign? which one does the magic?

The PURE comment.

You can also define component inside a function and add prop inside that and return the component with imediate function call and this would probably work.

const StyledBaseButton = /*#__PURE__*/ (() => { const MyComponent = () => {}; MyComponent.displayName = 'MyComponent'; return MyComponent; })();

The PURE comment only works on function calls so Object.assign just seems cleaner way compared to any other option.

anuraghazra · 2023-03-10T10:47:45Z

So here:

Why is the Input also got bundled when we are only importing Button?

saurabhdaware · 2023-03-13T03:42:38Z

Why is the Input also got bundled when we are only importing Button?

Input is getting bundled because we have this autocompleteSuggestionType array as separate variable. This might need some change in logic (maybe move this array inside component render or try something else) so I didn't get much into this right now. I'll create separate issue for input treeshaking.

burhanuday · 2023-03-13T04:11:53Z

Not sure why we are using the Pure pragma and not bundler level config? @saurabhdaware
Since most files will be side effects free, couldnt we use the sideEffects property in package json? I am sure webpack has this feature though not sure about Rollup. There is an issue referencing it here - rollup/rollup#2593 (comment)

saurabhdaware · 2023-03-13T04:37:29Z

sideEffects property goes on consumer's package.json usually. You can see in the issue you mentioned, rollup doesn't have sideEffects property support yet. I also tried adding sideEffects option to consumer with current latest blade but that didn't strip off Blade's bundle either 🤔

Also I am not sure if I would prefer that because that can cause unexpected results in production unless you have a good eslint setup that stops you from adding side-effects (which I couldn't find yet).

In my opinion, Ideal way to maintain treeshaking might be having a bundle-size check in PRs, having eslint rules to stop us from adding side-effects and a babel plugin to take care of common patterns 🤔

anuraghazra · 2023-03-13T07:37:20Z

Input is getting bundled because we have this autocompleteSuggestionType array as separate variable

I feel like something is wrong with our rollup setup or maybe with how the consumer end's config, because why is a variable which is defined as "const" treated as a side effect?

If we did something like "window.someVar = []" or "var someVar = []" then I could understand that.

Although we do transpile through babel which converts the const to var. so I think something can be done on the consumer's end.

@saurabhdaware can you push your testing setup on the /packages/example folder?

saurabhdaware · 2023-03-13T08:08:35Z

I feel like something is wrong with our rollup setup or maybe with how the consumer end's config, because why is a variable which is defined as "const" treated as a side effect?

Let me check this actually. I didn't try to fix it yet and assumed it is because of that variable (don't see anything else as of now)

Update: It might be because we're outputting ES5 but with ESM modules. I can see all variables turn into var in output.

anuraghazra · 2023-03-20T10:14:57Z

packages/blade/scripts/manualPureFunctions.js

+ * Once we upgrade to rollup 3.5.0, we can use treeshake.manualPureFunctions config from rollup instead of this plugin.
+ * https://rollupjs.org/configuration-options/#treeshake-manualpurefunctions
+ */
+const manualPureFunctions = () => ({


:p why is this "manual", shouldn't this be "auto"?

cause we manually define which functions should be pure 😆

This is what rollup follows - https://rollupjs.org/configuration-options/#treeshake-manualpurefunctions (we're just on older version so can't use this yet)

Can't we upgrade our rollup version and use this?

we can. It's a major version bump though so didn't spend much time right now. can pick it up next quarter

anuraghazra

LGTM, check bundling the code and do a sanity on the examples package.

saurabhdaware · 2023-03-21T05:14:42Z

LGTM, check bundling the code and do a sanity on the examples package.

Yup did. Working 👯

chaitanyadeorukhkar · 2023-03-21T10:37:23Z

I think its high time we add bundle size check on our PRs now 😅

saurabhdaware · 2023-03-21T10:39:37Z

I think its high time we add bundle size check on our PRs now 😅

Yes!! Definitely required!

chaitanyadeorukhkar · 2023-03-21T10:56:28Z

packages/blade/src/components/Card/CardHeader.tsx


-const CardHeaderIcon: WithComponentId<{ icon: IconComponent }> = ({ icon: Icon }) => {
+const _CardHeaderIcon = ({ icon: Icon }: { icon: IconComponent }): JSX.Element => {


curious why JSX.Element and not ReactNode or ReactElement

That is what we use generally for return type of React components so used the same

Let's revert to earlier React.ReactElement otherwise this may break typings for consumers in a patch release

I've been adding JSX.Element on all components I built 🙈 and there are few other instances of it in code.

I reverted the change here to how it was to not create breaking changes.

Created this issue to later change all JSX.Element instances to React.ReactElement #1089

divyanshu013

Looks good overall, one comment to make the return types same as earlier.

divyanshu013 · 2023-03-23T09:21:16Z

packages/blade/src/components/Card/CardHeader.tsx


-const CardHeaderIcon: WithComponentId<{ icon: IconComponent }> = ({ icon: Icon }) => {
+const _CardHeaderIcon = ({ icon: Icon }: { icon: IconComponent }): JSX.Element => {


Let's revert to earlier React.ReactElement otherwise this may break typings for consumers in a patch release

divyanshu013

* fix: tree-shaking in blade components * fix: displayName getting removed * feat: add assignWithoutSideEffects function * feat: remove .componentId assignments * feat: eslint rule * fix: baseinput test * Create .changeset/red-trees-hear.md * feat: revert to using React.ReactElement * fix: revert some JSX.Element changes * fix: revert ActionListHeader type

fix: tree-shaking in blade components

d29f01e

anuraghazra reviewed Mar 10, 2023

View reviewed changes

fix: displayName getting removed

601489f

saurabhdaware marked this pull request as ready for review March 10, 2023 09:54

saurabhdaware requested review from chaitanyadeorukhkar, kamleshchandnani, dhruvdutt, nashcheez and divyanshu013 as code owners March 10, 2023 09:54

divyanshu013 reviewed Mar 10, 2023

View reviewed changes

anuraghazra reviewed Mar 10, 2023

View reviewed changes

saurabhdaware added 6 commits March 13, 2023 18:18

feat: add assignWithoutSideEffects function

d7fe2a5

fix: resolve conflicts

5c08c14

feat: remove .componentId assignments

839a9c8

feat: eslint rule

6fca7bb

merge master

31770b3

fix: baseinput test

dae2761

saurabhdaware requested review from divyanshu013 and anuraghazra March 20, 2023 10:07

anuraghazra reviewed Mar 20, 2023

View reviewed changes

anuraghazra previously approved these changes Mar 20, 2023

View reviewed changes

saurabhdaware mentioned this pull request Mar 21, 2023

Performance Issue: Doesn't support tree shaking #959

Closed

chaitanyadeorukhkar reviewed Mar 21, 2023

View reviewed changes

chaitanyadeorukhkar previously approved these changes Mar 21, 2023

View reviewed changes

Create .changeset/red-trees-hear.md

dd61197

saurabhdaware dismissed stale reviews from chaitanyadeorukhkar and anuraghazra via dd61197 March 23, 2023 06:31

fix: conflicts

0781b27

saurabhdaware requested a review from chaitanyadeorukhkar March 23, 2023 06:37

saurabhdaware mentioned this pull request Mar 23, 2023

Upgrade to new rollup version #1087

Closed

chaitanyadeorukhkar previously approved these changes Mar 23, 2023

View reviewed changes

divyanshu013 reviewed Mar 23, 2023

View reviewed changes

feat: revert to using React.ReactElement

9772fbe

saurabhdaware dismissed chaitanyadeorukhkar’s stale review via 9772fbe March 23, 2023 09:35

saurabhdaware added 2 commits March 23, 2023 15:10

fix: revert some JSX.Element changes

da5f6d6

fix: revert ActionListHeader type

4cda6f6

saurabhdaware requested review from divyanshu013 and chaitanyadeorukhkar March 23, 2023 09:46

divyanshu013 approved these changes Mar 23, 2023

View reviewed changes

saurabhdaware merged commit fe89e6f into master Mar 23, 2023
5 checks passed

saurabhdaware deleted the saurabh/fix/tree-shaking branch March 23, 2023 11:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: tree-shaking in blade components #1045

fix: tree-shaking in blade components #1045

saurabhdaware commented Mar 10, 2023 •

edited

changeset-bot bot commented Mar 10, 2023 •

edited

codesandbox-ci bot commented Mar 10, 2023 •

edited

anuraghazra Mar 10, 2023

saurabhdaware Mar 10, 2023

github-actions bot commented Mar 10, 2023 •

edited

divyanshu013 Mar 10, 2023

divyanshu013 Mar 10, 2023

divyanshu013 Mar 10, 2023

saurabhdaware Mar 13, 2023

saurabhdaware Mar 13, 2023

anuraghazra Mar 10, 2023

saurabhdaware Mar 13, 2023

anuraghazra commented Mar 10, 2023

saurabhdaware commented Mar 13, 2023

burhanuday commented Mar 13, 2023

saurabhdaware commented Mar 13, 2023

anuraghazra commented Mar 13, 2023

saurabhdaware commented Mar 13, 2023 •

edited

anuraghazra Mar 20, 2023

saurabhdaware Mar 20, 2023

chaitanyadeorukhkar Mar 21, 2023

saurabhdaware Mar 23, 2023

anuraghazra left a comment

saurabhdaware commented Mar 21, 2023

chaitanyadeorukhkar commented Mar 21, 2023

saurabhdaware commented Mar 21, 2023

chaitanyadeorukhkar Mar 21, 2023

saurabhdaware Mar 23, 2023

divyanshu013 Mar 23, 2023

saurabhdaware Mar 23, 2023

divyanshu013 left a comment

divyanshu013 Mar 23, 2023

divyanshu013 left a comment


		const CardHeaderIcon: WithComponentId<{ icon: IconComponent }> = ({ icon: Icon }) => {
		const _CardHeaderIcon = ({ icon: Icon }: { icon: IconComponent }): JSX.Element => {

fix: tree-shaking in blade components #1045

fix: tree-shaking in blade components #1045

Conversation

saurabhdaware commented Mar 10, 2023 • edited

Description

Before

After

Issue

😇 This creates no side-effects

😇 This creates no side-effects

👿 This CREATES side-effects

😇 Solution - This creates no side-effects

changeset-bot bot commented Mar 10, 2023 • edited

🦋 Changeset detected

codesandbox-ci bot commented Mar 10, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Mar 10, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anuraghazra commented Mar 10, 2023

saurabhdaware commented Mar 13, 2023

burhanuday commented Mar 13, 2023

saurabhdaware commented Mar 13, 2023

anuraghazra commented Mar 13, 2023

saurabhdaware commented Mar 13, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anuraghazra left a comment

Choose a reason for hiding this comment

saurabhdaware commented Mar 21, 2023

chaitanyadeorukhkar commented Mar 21, 2023

saurabhdaware commented Mar 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

divyanshu013 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

divyanshu013 left a comment

Choose a reason for hiding this comment

saurabhdaware commented Mar 10, 2023 •

edited

changeset-bot bot commented Mar 10, 2023 •

edited

codesandbox-ci bot commented Mar 10, 2023 •

edited

github-actions bot commented Mar 10, 2023 •

edited

saurabhdaware commented Mar 13, 2023 •

edited