Refactor React components to fall more inline with existing React libraries

[eagerestwolf]

Describe the feature in detail (code, mocks, or screenshots encouraged)

So, the time has come. I am going to open this issue as a place to track whats going on, and to communicate changes and whatnot before opening a full fledged pull request. I am by no means a React expert (though I follow one on YouTube lol), with that said the goal of this issue/pull request is going to be manyfold:

• First, optimize the React components. Running the React app right now, it's using a pretty healthy chunk of my CPU (and I have a Ryzen 5 5600X); so there are probably some unnecessary re-renders happening throughout the code base (and by that I mean there are, I have been starting on the rewrite already).
• Second, bring the library more inline with what the React community is largely already doing, thus making the library feel easy to use (and debug if needed) for React developers.
• Third, try to optimize compatibility with as many frameworks as possible, including vanilla React, NextJS, Remix, Quik, React Router v7 aka Remix v3 (once I can start testing that), Preact, SolidJS (don't get your hopes up, I just want to make as many components as possible work for you guys), and (of course) Astro.

With that said, I have made one change already (that I feel is 100% necessary, fight me) and that is adding a path alias to the tsconfig.json for the ./src/* path (of course it's @/*, fight me again). However, I do have a few proposed changes that need to be run by the Skeleton team before I implement them:

• First, return types for functional components in React are generally a no-no. Hear me out on this one. Generally (especially for component libraries), it's best to let TypeScript's type inference to the work it's magic. As an example, this is a component file from Theo Browne's UploadThing. You'll notice that he doesn't use return types anywhere. While UploadThing's case is a little different, since it's not a component library, you'll notice that Radix also doesn't have return types. For a simple component library like Skeleton, this has a few advantages. First, it allows the team to pass the task of optimization from the library to the consumer. This is generally a good thing because everyone likes to optimize in a different way (some like Babel, others like Vite), and it's best to allow consumers to develop a workflow that works well for them. Secondly, not typing the returns allows some components (those that don't use anything from React, like the Avatar component) to be used with other frameworks like Preact (although Preact does have a React compatibility library, so they can use the full thing) and SolidJS. Which brings me onto my second point.
• Second, by changing jsx from react-jsx to preserve you not only enable that consumer optimization I was referring to earlier, you also enable other frameworks (again, where other React imports aren't used). However, that comes with a major drawback. You cannot have the main React app that renders the React components within the React component library itself. Which brings me to my third point.
• Third, it's generally not a good idea to ship an entire React app in a component library. I understand why it's done in this case. That React app is later passed onto Astro and used as the basis for the React docs. I am not super familiar with Astro, but I am almost certain there is a way to do this in Astro without needing to bundle the entire React application in the component library. Component libraries and apps have very different requirements, and shipping useless code to consumers is generally not a good idea.
• Fourth, I can't find any direct examples of this, but a lot of React developers have switched to function declarations as opposed to arrow functions for TypeScript. As far as I know, on the JavaScript side, it's still mostly arrow functions, but who uses vanilla JS these days? From someone else who's been poking around the web development scene since Airbnb was popular, I get it, old habits are hard to break; but that's just what most people are doing these days. It's a personal preference, I'll do whatever the team wants. Both are literally the exact same thing under the hood.
• Fifth, generally props are defined in the same file as the component. However, I understand why you have a separate types file, so I'll leave that alone. The part that is a big deal is contexts. Contexts are almost always defined in their own file. Why? I dunno, that's just what React developers do. Probably separation of concerns, keep logic out of the UI layer, and all that. So, for any component (such as the Accordion) that defines a context, that should probably be in a context.ts file and imported.

That's all I can think of at the moment, but I am working on the React rewrite as quickly as I can. As an example, here is what the Avatar component would look like before and after

BEFORE

'use client';

import React from 'react';
import { AvatarProps } from './types.js';

export const Avatar: React.FC<AvatarProps> = ({
	src = '',
	alt = '',
	filter = '',
	// Root
	base = 'overflow-hidden isolate',
	background = 'bg-surface-400-600',
	size = 'size-16',
	font = '',
	border = '',
	rounded = 'rounded-full',
	shadow = '',
	classes = '',
	// Image
	imageBase = 'w-full object-cover',
	imageClasses = '',
	// Children
	children
}) => {
	return (
		<figure className={`${base} ${background} ${size} ${font} ${border} ${rounded} ${shadow} ${classes}`} data-testid="avatar">
			{src ? (
				<img className={`${imageBase} ${imageClasses}`} src={src} alt={alt} style={{ filter: filter ? `url(${filter})` : undefined }} />
			) : (
				children
			)}
		</figure>
	);
};

AFTER

import { AvatarProps } from './types.js';

export function Avatar({
	src = '',
	alt = '',
	filter = '',
	// Root
	base = 'overflow-hidden isolate',
	background = 'bg-surface-400-600',
	size = 'size-16',
	font = '',
	border = '',
	rounded = 'rounded-full',
	shadow = '',
	classes = '',
	// Image
	imageBase = 'w-full object-cover',
	imageClasses = '',
	// Children
	children
}: Readonly<AvatarProps>) {
	return (
		<figure className={`${base} ${background} ${size} ${font} ${border} ${rounded} ${shadow} ${classes}`} data-testid="avatar">
			{src ? (
				<img className={`${imageBase} ${imageClasses}`} src={src} alt={alt} style={{ filter: filter ? `url(${filter})` : undefined }} />
			) : (
				children
			)}
		</figure>
	);
};

For a more complex example, here's a before and after of the AppBar component

BEFORE

import React from 'react';
import { ToolbarCenterProps, AppBarHeadlineProps, ToolbarLeadProps, AppBarProps, ToolBarProps, ToolbarTrailProps } from './types.js';

// React Compose ---
import { reactCompose } from '../../utils/react-compose.js';

// Components ---
const AppBarRoot: React.FC<AppBarProps> = ({
	// Root
	base = 'w-full flex flex-col',
	background = 'bg-surface-100-900',
	spaceY = 'space-y-4',
	border = '',
	padding = 'p-4',
	shadow = '',
	classes = '',
	// Children
	children
}) => {
	return (
		<div className={`${base} ${background} ${spaceY} ${border} ${padding} ${shadow} ${classes}`} role="toolbar" data-testid="app-bar">
			{children}
		</div>
	);
};

const Toolbar: React.FC<ToolBarProps> = ({
	// Toolbar
	base = 'flex justify-between',
	gridCols = 'grid-cols-[auto_1fr_auto]',
	gap = 'gap-4',
	classes = '',
	// Children
	children
}) => {
	return <div className={`${base} ${gridCols} ${gap} ${classes}`}>{children}</div>;
};

const ToolbarLead: React.FC<ToolbarLeadProps> = ({
	// Lead
	base = 'flex',
	spaceX = 'space-x-4 rtl:space-x-reverse',
	padding = '',
	classes = '',
	// Children
	children
}) => {
	return <div className={`${base} ${spaceX} ${padding} ${classes}`}>{children}</div>;
};

const ToolbarCenter: React.FC<ToolbarCenterProps> = ({
	// Center
	base = 'grow',
	align = 'text-center',
	padding = '',
	classes = '',
	// Children
	children
}) => {
	return <div className={`${base} ${align} ${padding} ${classes}`}>{children}</div>;
};

const ToolbarTrail: React.FC<ToolbarTrailProps> = ({
	// Trail
	base = 'flex',
	spaceX = 'space-x-4 rtl:space-x-reverse',
	padding = '',
	classes = '',
	// Children
	children
}) => {
	return <div className={`${base} ${spaceX} ${padding} ${classes}`}>{children}</div>;
};

const AppBarHeadline: React.FC<AppBarHeadlineProps> = ({
	// Headline
	base = 'w-full',
	classes = '',
	// Children
	children
}) => {
	return <div className={`${base} ${classes}`}>{children}</div>;
};

export const AppBar = reactCompose(AppBarRoot, {
	Toolbar: Toolbar,
	ToolbarLead: ToolbarLead,
	ToolbarCenter: ToolbarCenter,
	ToolbarTrail: ToolbarTrail,
	Headline: AppBarHeadline
});

AFTER

import { ToolbarCenterProps, AppBarHeadlineProps, ToolbarLeadProps, AppBarProps, ToolBarProps, ToolbarTrailProps } from './types.js';

// React Compose ---
import { reactCompose } from "@/lib/utils/react-compose.js";

// Components ---
function AppBarRoot({
	// Root
	base = 'w-full flex flex-col',
	background = 'bg-surface-100-900',
	spaceY = 'space-y-4',
	border = '',
	padding = 'p-4',
	shadow = '',
	classes = '',
	// Children
	children
}: Readonly<AppBarProps>) {
	return (
		<div className={`${base} ${background} ${spaceY} ${border} ${padding} ${shadow} ${classes}`} role="toolbar" data-testid="app-bar">
			{children}
		</div>
	);
}

function Toolbar({
	// Toolbar
	base = 'flex justify-between',
	gridCols = 'grid-cols-[auto_1fr_auto]',
	gap = 'gap-4',
	classes = '',
	// Children
	children
}: Readonly<ToolBarProps>) {
	return <div className={`${base} ${gridCols} ${gap} ${classes}`}>{children}</div>;
};

function ToolbarLead({
	// Lead
	base = 'flex',
	spaceX = 'space-x-4 rtl:space-x-reverse',
	padding = '',
	classes = '',
	// Children
	children
}: Readonly<ToolbarLeadProps>) {
	return <div className={`${base} ${spaceX} ${padding} ${classes}`}>{children}</div>;
};

function ToolbarCenter({
	// Center
	base = 'grow',
	align = 'text-center',
	padding = '',
	classes = '',
	// Children
	children
}: Readonly<ToolbarCenterProps>) {
	return <div className={`${base} ${align} ${padding} ${classes}`}>{children}</div>;
};

function ToolbarTrail({
	// Trail
	base = 'flex',
	spaceX = 'space-x-4 rtl:space-x-reverse',
	padding = '',
	classes = '',
	// Children
	children
}: Readonly<ToolbarTrailProps>) {
	return <div className={`${base} ${spaceX} ${padding} ${classes}`}>{children}</div>;
};

function AppBarHeadline({
	// Headline
	base = 'w-full',
	classes = '',
	// Children
	children
}: Readonly<AppBarHeadlineProps>) {
	return <div className={`${base} ${classes}`}>{children}</div>;
};

export const AppBar = reactCompose(AppBarRoot, {
	Toolbar,
	ToolbarLead,
	ToolbarCenter,
	ToolbarTrail,
	Headline: AppBarHeadline
});

What type of pull request would this be?

Other

Provide relevant links or additional information.

Side note, import React from 'react' is no longer needed since I switched the project to using the new JSX Transform when ESLint and Prettier got taken out of the packages, so unless you are using the React namespace for something else, it doesn't need to be imported anymore.

Also, I removed the .js extensions from the imports. It's not needed when using TypeScript, and everything works fine without it.

Also also, I had to rollback the tsconfig paths for the react package. It broke the docs site. Apparently, Vite and vite-plugin-pagefind do not play nicely with tsconfig files.

[endigo9740]

Thanks @eagerestwolf, two things of note.

First off, make sure you've read through this post. This outlines my availability, especially this week:

• Priority updates for Skeleton Labs #2729

Second, per this

Third, it's generally not a good idea to ship an entire React app in a component library.

This is a known issue and already on our radar to resolve. It was our first time packaging a Vite/React app for NPM and we mistakenly shipped more than just the components.

I'm interested in learning more about the benefits of the syntactical changes you mentioned, but performance should be priority number one. We've made things work, now we make them work better. That's where I assumed we had the biggest issue and what I'd like to tackle first. As you mention, it's likely due to re-rendering and/or general memoization issues.

My suggestion is we tackle each of the issues brought up above in individual dedicated PRs if possible. Handle it in "waves". That way we can keep changes small, surgical, and isolated.

No rush if you wish to help contribute directly, especially given I'll be out for the next week, but I look forward to tackling these issues soon.

[Hugos68]

Hey @eagerestwolf, first of all, thanks for the write up! I am one of the people that helped authoring the React components (Accordion and Progress). So any tips are genuinly much appreciated since I have no prior experience with React (apart from watching the occational Theo video).

Also you stated:

Also also, I had to rollback the tsconfig paths for the react package. It broke the docs site. Apparently, Vite and vite-plugin-pagefind do not play nicely with tsconfig files.

I am the author of vite-plugin-pagefind and I am very interested in what you mean with "do not play nicely with tsconfig files" since the plugin never interacts with any typescript stuff as it's just a Vite plugin. I would love to fix this issue if it turns out to be an actual bug.

You also said:

Also, I removed the .js extensions from the imports. It's not needed when using TypeScript, and everything works fine without it.

I'm not sure I agree with this, I had learned that .js actually helps improve performance during development/build but maybe that's not relevant anymore, we should investigate this a bit further.

Like Chris said, I think opening a seperate issue and PR for each problem is best where we can deepdive into each problem in isolation.

[eagerestwolf]

So, the syntactical changes (other than removing the return types) have literally no performance impact whatsoever.

@Hugos68, I do apologize, I actually misspoke to some extent, I remembered like right before I went to sleep exactly what I was doing wrong when it comes to paths. You are correct in that Vite doesn't do anything with tsconfig files. In fact, per the Vite docs, it really take the tsconfig.json into account. I forgot that Vite has it's own paths parameter. I should have known about that from my limited Svelte development. On the React side, I've always used Webpack (until recently) since that was the standard on the React side for so long, and since you guys are using "moduleResolution": "bundler", Vite would be the correct place to do that anyway. As for the .js extensions. Oh boy, is that a can of worms. I never understood how deep that issue is since I have never encountered it, but there are a ton of issues open in the nodejs/node and microsoft/TypeScript repos for that. I think the best issue to highlight the problem, which actually links to a few others though is nodejs/node#46006. With that said, it probably is best to just leave the .js extensions in place since they don't really hurt anything.

With that said, I guess my plan of action for the time being is to set up the path alias with both Vite and TypeScript (Vite for building, and TypeScript for development) and switch everything over to using the reactCompose helper. Then, I can work on refactoring components to optimize performance. However, I will open a separate issue for all of that and just link the issues back to this. I guess we can use this issue to track the overall progress and use the smaller issues and PRs to track the individual changes.

For anyone interested in what the React best practices look like, there are 3 important things to be familiar with:

• the Rules of Hooks (which is enforced by eslint-plugin-react-hooks)
• the Rules of React (which is enforced by eslint-plugin-react-compiler)
• the Server and Client Composition Patterns (not enforced by ESLint, so you really just need to know these)

[endigo9740]

Quick update per the audit items above - we've merged the following PR:

• Remove reactCompose utility #2734

This does two thigns:

1. Sets the proper $lib alias path in the skeleton-react project
2. Replaces the reactCompose function with direct use of Object.assign

Going forward we should ensure all work conforms to use these.