Skip to content

introduced model interface IPerson #2

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.

Sign up for GitHub

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

Open
marcwittke wants to merge 1 commit into
base: main
Choose a base branch
from
Open

introduced model interface IPerson #2

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 4 additions & 8 deletions src/App.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,20 +3,16 @@ import logo from './logo.svg';
import './App.css';
import List from './components/List';
import AddToList from './components/AddToList';
import {IPerson} from "./models/IPerson";

export interface IState {
people: {
name: string
age: number
img: string
note?: string
}[]
interface IState {
people: IPerson[]
}


function App() {

const [people, setPeople] = useState<IState["people"]>([
const [people, setPeople] = useState<IPerson[]>([
{
name: "LeBron James",
age: 35,
Expand Down
9 changes: 5 additions & 4 deletions src/components/AddToList.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
import React, { useState } from 'react'
import { IState as Props } from "../App";
import {IPerson} from "../models/IPerson";


interface IProps {
setPeople: React.Dispatch<React.SetStateAction<Props["people"]>>
people: Props["people"]
setPeople: React.Dispatch<React.SetStateAction<IPerson[]>>
Copy link

@PhilippSpo PhilippSpo Feb 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using React.Dispatch<React.SetStateAction<>> already introduces coupling as we are basically saying that it must be a native react state setter the needs to be passed in here. But what if we are not using react state in the parent component?

We could, instead, define what the AddToList component actually expects: A function that takes a list of persons and returns nothing:

Suggested change
setPeople: React.Dispatch<React.SetStateAction<IPerson[]>>
setPeople: (persons: IPerson[]) => void

people: IPerson[]
}

const AddToList: React.FC<IProps> = ({setPeople, people}) => {
Expand Down Expand Up @@ -32,7 +33,7 @@ const AddToList: React.FC<IProps> = ({setPeople, people}) => {
age: parseInt(input.age),
img: input.img,
note: input.note
}
} as IPerson
Copy link

@PhilippSpo PhilippSpo Feb 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should avoid type assertions if we can. It basically tells typescript "I know more than you do about this type", with the risk of being wrong 😄 .

We shouldn't need to tell typescript about the type of this object, as the setPeople handler only accepts People to be set.

Copy link
Author

@marcwittke marcwittke Feb 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as a dotnetter I always shudder when I see as, too. But, here it is the creation of an instance - is there a better way to make an instance of an interface?

Copy link

@PhilippSpo PhilippSpo Feb 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is not really such thing in typescript due to structural typing. If an object has the same shape as the IPerson interface you can consider it an instance of it.

But when you create a variable and want it to explicitly be of type IPerson you can declare it like so:

const person: IPerson = {
    name: "Marc",
    age: 1,
    img: "foo"
}

(playground example)

]);

setInput({
Expand Down
4 changes: 2 additions & 2 deletions src/components/List.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
import React from 'react'
import { IState as Props } from "../App";
import {IPerson} from "../models/IPerson";

interface IProps {
people: Props["people"]
people: IPerson[]
}

const List: React.FC<IProps> = ({ people }) => {
Expand Down
6 changes: 6 additions & 0 deletions src/models/IPerson.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
export interface IPerson {
Copy link

@PhilippSpo PhilippSpo Feb 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would agree, the more we are using this type across our codebase, the more it makes sense to extract it to a separate location.

I totally agree with the coupling argument. The AddToList component should not be coupled to be used inside the List component. It should not care where it is used.

The question is rather, should the AddToList component be coupled to the general purpose IPerson interface? Or should it define its own type?

One could argue, that as a presentational component, it defines its own type. It renders four input fields no matter how the IPerson interface changes. Then if the IPerson interface changes, we wouldn't necessarily need to change the source code of the AddToList component, but could change how we pass the props to it in the List.

On the other hand, one could say that in this specific app, it makes sense that whenever we change the IPerson interface, the AddToList component should change with it.

I hope this made sense 😅

name: string;
age: number;
img: string;
note?: string;
}