I've noticed a few things, which I'm going to elaborate in the following.
Interfaces
First off, your interface Expense
.
It looks good so far, what I'd like to see is something like a structure in it, let it be alphabetically sorted or sort it optionals first or both, something like:
export interface Expense {
amount: number;
createdAt: number;
description: string;
id: string;
note?: string;
}
It increases readability and allows others to quickly parse all existing keys (it's a bit nitpicking here, but it can have a huge effect).
Next up:
type PathParamsType = {
id: string;
};
//component own props
type PropsType = RouteComponentProps<PathParamsType> & {
id: string;
};
interface StateProps {
expenses: Expense[]
}
interface DispatchProps{
addExpense : (expense : Expense) => any,
removeExpense : (expenseId : string) => any
}
type Props = PathParamsType & PropsType & StateProps & DispatchProps;
My two pain points here are, PathParamsType
as well as DispatchProps
.
PathParamsType is not speaking for it self, try to give variables or interfaces a very consice name. Also I thought, you accidentially declared { id: string; }
twice. There is some kind of repetition that could have been avoided. Please consider the following:
interface WithId {
id: string;
}
/* Maybe name ComponentProps to something more speaking. */
type ComponentProps = RouteComponentProps<WithId> & WithId;
Also, you are declaring a props type below, why didn't you merge all in the first place.
Something along the lines like: type Props = RouteComponentProps<WithId> & WithId /* & so on ... */;
.
!!Side note!! Avoid stuff like: type Foo = { bar: string; foo: number; }
we have interfaces for that.
Your function typing seems also a bit odd to me.
interface DispatchProps{
addExpense : (expense : Expense) => any,
removeExpense : (expenseId : string) => any
}
any
seems in appropriate here. If you don't return a value from a function use void
. If that function never returns (e.g. you quit the application inside that function), use the keyword never
. But please don't use any
.
To sum the interface section up, your code from above could have looked a bit like this:
interface WithId {
id: string;
}
interface StateProps {
expenses: Expense[];
}
interface DispatchProps{
addExpense : (expense : Expense) => void;
removeExpense : (expenseId : string) => void;
}
type Props = RouteComponentProps<WithId> & WithId & StateProps & DispatchProps;
React Component
Let's get to the component itself.
const EditExpensePage: React.FunctionComponent<Props> = (props): any => {
/* ... */
}
The same as in the DispatchProps
interface, a function does not return any. You actually broke the type system here. The following is fully compliant to your function/ variable typing.
const IamBroken: React.FunctionComponent<Props> = (props): any => 5;
This works as IamBroken should be a functional component but the function itself returns any and any is 5 (hopefully that explanation helps, at least a bit). What you would have wanted instead is:
const IamNotBroken = (props): React.FunctionComponent<Props> => 5; /* TypeScript: ERROR!!!! */
To apply the above pattern to your component (two solutions are available):
/* Variant 1 */
const EditExpensePage = (props: Props) => {
/* ... */
}
/* Variant 2 */
const EditExpensePage: React.FunctionComponent<Props> = (props) => {
/* ... */
}
Both solutions are totally fine, if you ask me though, I prefer the first one, it's sleek and if needed the compiler is able to infere the React.FunctionComponent
part.
Component internals, functional programming
Inside your component I've noticed a few things as well.
let selectedExpense = GetSelectedExpense(props.match.params.id, props.expenses)
. From what I have seen, that you never re-assign selectedExpense. Therefore it's better to keep such variables as constants: const selectedExpense
.
I see what you want to achieve with the GetSelectedExpense function. But there are better way's to achieve this in TypeScript/JavaScript. Have a look at Arrays over at MDN. There are a few helpful methods for overall array handling. What you are looking for is something like find
.
(Side Note: There is a powerful concept that is called destructuring, and it helps you to avoid to write props.
over and over again).
const { expenses, match } = props; /* Concept of destructuring, see my side note above */
const selectedExpense: Expense | undefined = expenses.find(expense => expense.id === match.params.id);
The onSubmit
function could be improved in the following way. First, you want to avoid inline array functions as props (many linters are warning about possible performance issues). While that seems to be true, it also doesn't add to readability as well. Have a look at the following (I'm making use of the useCallback hook):
const { expenses, selectedExpense, removeExpense, addExpense, match } = props;
const selectedExpense: Expense | undefined = expenses.find(expense => expense.id === match.params.id);
const onSubmit = useCallback((expense) => {
expenses.filter(exp => exp.id === selectedExpense).map(exp => removeExpense(exp.id));
addExpense(expense);
}, [removeExpense, addExpense, selectedExpense]);
return (<ExpenseForm description={selectedExpense?.description} amount={selectedExpense?.amount} onSubmit={onSubmit}/>);
Last but not least, your compose thingy at the end. It seems wrong and should be like this (because compose
is coming from redux which shouldn't be abused to chain higher order components):
export default withRouter(connect(mapStateToProps, mapDispatchToProps)(EditExpensePage));