Narrow types with TypeScript instead of erroring There are many places in the code where you check a condition based on how the Button is being called, and if it isn't fulfilled, you log an error and return nothing. This is a reasonable approach in JavaScript, but in TypeScript, you have a much better option, which is to use types to require only certain arguments. This way, rather than an error being thrown at runtime, an error will be thrown by TypeScript at compile-time when a user of Button
attempts to pass props that aren't valid. Turning typos / accidental incorrect function calls into compile-time errors is one of the greatest advantages of TypeScript, and it's great that you're using TypeScript already, so go ahead and take advantage of it.
For example, rather than:
const availableColors = ['blue', 'grey', 'dark-grey', 'white'];
const borderColors = [''];
let buttonStyle;
// Check if props.color exists in availableColors
if(!availableColors.includes(props.color)) {
console.error(`"${props.color} color key not available for button style`);
return;
}
You can make availableColors
as a type instead, and require that the color
prop matches it:
'blue' | 'grey' | 'dark-grey' | 'white'
Similarly, rather than
if(!props.content) {
console.error(`"${props.content} is not valid as content`);
you can require that the content
property is of type:
T & (T extends '' ? 'Content must not be empty' : {})
and instead of
if(props.type !== 'link') {
require a type
of 'link' | ''
exactly.
Using the above methods will also let you declare buttonStyle
and elementType
with const
- or you could even inline their values into the props.
Default export? You have:
export default function Button
This will mean that the imported button is not really tied to its Button
name anymore. For example, it would be easy for someone to initially type:
import button from './Button';
only to run into problems since they forgot to capitalize it. Using named exports makes these sorts of typos more difficult, because one has to intentionally rename it to get something other than Button:
// Natural import
import { Button } from './Button';
or
// Deliberate renaming
import { Button as RenamedVariableName } from './Button';
Destructure immediately Rather than referencing props
multiple times in the function body, you might consider extracting the properties from the props immediately in the function definition - it makes the logic later less noisy.
Let TS infer types when possible There's no need to note that a function returns a particular type if
- TypeScript can already infer the type automatically, and
- The type is obvious to a reader of a code at a glance
So, you may consider removing the JSX.Element
from (props: Props): JSX.Element
.
buttonStyles
? It sounds like buttonStyles
is an object that contains strings, where the strings are class names. So buttonStyles
isn't a very precise variable name - it doesn't contain styles, it contains class names. Maybe instead call it buttonClassNamesByColor
?
Fragments In newer versions of React, you may use <>
and </>
instead of <React.Fragment>
</React.Fragment>
. Or, even better, since you're returning only a single JSX element regardless, you can use the conditional operator to return either one or the other element, without wrapping everything in a fragment.
export const Button = <T extends string>({
content,
color,
path,
type,
}: {
content: T & (T extends '' ? 'Content must not be empty' : {});
color: 'blue' | 'grey' | 'dark-grey' | 'white';
path?: string;
type: '' | 'link';
}) => {
const buttonClassName = buttonClassNamesByColor[`button--${color}`];
return type === ''
? <button className={buttonClassName}>{content}</button>
: <Link to={path} className={buttonClassName}>{content}</Link>;
}