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

Bug - [react-table] - Using a Table component causes all of PatternFly's CSS to be imported #10618

Open
jelly opened this issue Jun 17, 2024 · 4 comments
Labels
Milestone

Comments

@jelly
Copy link
Contributor

jelly commented Jun 17, 2024

Describe the problem

We have been using esbuild for a while now with Cockpit, and recently we found that it includes a bundle analyzer. Our bundle is great large (uncompressed) so it would be interesting to see if we can make it smaller.

(If you want to see this in action, upload this meta.json file to the esbuild bundle analyzer).

What stands out is the 1.5 mb of react-styles which are imported, this project (cockpit-machines) does not use a Masthead component but the css is still imported.

image

The bundle analyzer can also show why something is imported

image

So importing Table/index.js imports

import { useOUIAProps, handleArrows, setTabIndex } from '@patternfly/react-core';

The problem here is that @patternfly/react-core is a barrel file, so importing it this way imports @patternfly/react-core/dist/js/index.js:

[jelle@t14s][~/projects/cockpit-machines]%cat -p node_modules/@patternfly/react-core/dist/esm/index.js
export * from './components';
export * from './layouts';
export * from './helpers';
export * from './styles';
//# sourceMappingURL=index.js.map

So basically it imports all of PatternFly, esbuild seems to be smart enough to not include unused JavaScript but it seems it doesn't handle CSS.

An easy fix would be for react-table to import for example setTabIndex from react-core/dist/js/helpers/KeyboardHandler.js directly.

@martinpitt
Copy link

In fact we do this a lot in our projects: We never import the full @patternfly/react-core', but only the components which we need, like in https://github.com/cockpit-project/cockpit-machines/blob/main/src/components/networks/network.jsx:

import { Button } from "@patternfly/react-core/dist/esm/components/Button";
import { DropdownItem } from "@patternfly/react-core/dist/esm/components/Dropdown";
import { Tooltip } from "@patternfly/react-core/dist/esm/components/Tooltip";

@jelly
Copy link
Contributor Author

jelly commented Jun 17, 2024

I looked at react-table and edited our node_modules to see if it would make an impact:

diff --git a/@patternfly/react-table/dist/esm/components/Table/Table.js b/@patternfly/react-table/dist/esm/components/Table/Table.js
index 702ab38a2d..e6c40d5ace 100644
--- a/@patternfly/react-table/dist/esm/components/Table/Table.js
+++ b/@patternfly/react-table/dist/esm/components/Table/Table.js
@@ -5,7 +5,8 @@ import stylesGrid from '@patternfly/react-styles/css/components/Table/table-grid
 import stylesTreeView from '@patternfly/react-styles/css/components/Table/table-tree-view.mjs';
 import { css } from '@patternfly/react-styles';
 import { toCamel } from './utils';
-import { useOUIAProps, handleArrows, setTabIndex } from '@patternfly/react-core';
+import { handleArrows, setTabIndex } from '@patternfly/react-core/dist/esm/helpers/KeyboardHandler';
+import { useOUIAProps } from "@patternfly/react-core/dist/esm/helpers/OUIA/ouia";
 import { TableGridBreakpoint } from './TableTypes';
 export const TableContext = React.createContext({
     registerSelectableRow: () => { }

Old meta.json
New meta.json

In numbers the result is:
8 mb input size => 4.7 mb output size
6.7 mb input size => 4.0 mb output size

This would be a small but nice reduction in final bundle size for cockpit-ostree, for cockpit-machines I would need to patch more PF components to get similar results.

Old:
image

New:
image

martinpitt added a commit to cockpit-project/cockpit-files that referenced this issue Jun 17, 2024
This does not currently make any difference for the dist/ size because
of patternfly/patternfly-react#10618. But we
do that in all other projects.
martinpitt added a commit to cockpit-project/cockpit-files that referenced this issue Jun 17, 2024
This does not currently make any difference for the dist/ size because
of patternfly/patternfly-react#10618. But we
do that in all other projects.
martinpitt added a commit to cockpit-project/cockpit-files that referenced this issue Jun 18, 2024
This does not currently make any difference for the dist/ size because
of patternfly/patternfly-react#10618. But we
do that in all other projects.
jelly pushed a commit to cockpit-project/cockpit-files that referenced this issue Jun 18, 2024
This does not currently make any difference for the dist/ size because
of patternfly/patternfly-react#10618. But we
do that in all other projects.
@tlabaj tlabaj moved this from Needs triage to Backlog in PatternFly Issues Aug 8, 2024
@tlabaj tlabaj added this to the Prioritized Backlog milestone Aug 8, 2024
Copy link

github-actions bot commented Oct 9, 2024

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@jelly
Copy link
Contributor Author

jelly commented Oct 9, 2024

Need to recheck with PF6 but this is still an issue in PF5.

@github-actions github-actions bot removed the wontfix label Oct 10, 2024
@kmcfaul kmcfaul removed the bug label Oct 25, 2024
@tlabaj tlabaj added the PF5 label Nov 5, 2024
@tlabaj tlabaj moved this from Backlog to Ready to assign in PatternFly Issues Nov 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Ready to assign
Development

No branches or pull requests

4 participants