The components folder has been converted to TypeScript. #650

TheMaja · 2023-05-15T06:41:17Z

Related issues: [please specify]

Description:

What are you up to? Fill us in :)

Screenshots/videos

I solemnly swear that:

My code follows the style guidelines of this project
I have merged the develop branch into my branch and fixed any merge conflicts
I have performed a self-review of my own code
I have commented my code in hard-to-understand areas
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
I have tested new or existing tests and made sure that they pass

sheenarbw

This is a good start but has a few rough edges:

Always try to be explicit:

Use of any. Please try not to use any. Rather be explicit. Otherwise when a new coder works on this stuff they will be encouraged to guess and there will be errors that come from that.
Use of ReactNode when we can refer to a specific type.

Make sure you aren't making changes you don't meant to make. There are a few strange things in here.

I haven't actually run the code and looked for errors. Can you please check yourself for errors by running

npm run build
the storybook tests

The storybook tests use this https://storybook.js.org/docs/react/writing-tests/test-runner

You can run them by opening two terminals:

# terminal 1

npm run storybook

# terminal 2

npm run test-storybook

sheenarbw · 2023-05-15T06:57:19Z

frontend-mini-challenge/components/LoggedOutPage.tsx

+      padding="md"
+      header={
+        <Header height={60} p="xs">
+          <Container></Container>


Why is there a container in the header now?

sheenarbw · 2023-05-15T07:00:55Z

frontend-mini-challenge/components/LoggedInPage.tsx

+  children: React.ReactNode;
+  serverSidePropsCorrectlyCalled: boolean;
+  isLoggedIn: boolean;
+  loggedInUserData: any;


loggedInUserData shouldn't be any

sheenarbw · 2023-05-15T07:02:44Z

frontend-mini-challenge/components/LoggedInPage.tsx

+  query,
+  req,
+}: {
+  query: any;


These shoulnd't be any

sheenarbw · 2023-05-15T07:03:13Z

frontend-mini-challenge/components/LoggedInPage.tsx

+  query: any;
+  req: any;
+}) {
+  const whoAmIResponse = await serverSideWhoAmI({ query, req } as any);


also not any

sheenarbw · 2023-05-15T07:05:07Z

frontend-mini-challenge/tsconfig.json

    "incremental": true
  },
-  "include": ["next-env.d.ts", "**/*.ts", "**/*.tsx"],
+  "include": ["next-env.d.ts", "**/*.ts", "**/*.tsx", "components/LoggedOutPage.tsx"],


Look at the existing patterns here. Do you think "components/LoggedOutPage.tsx" is actually helpful? Or do you think it is already included?

sheenarbw · 2023-05-15T07:06:18Z

frontend-mini-challenge/components/Text.tsx

 }

-export function Underlined({ children }) {
+export function Underlined({ children }: { children: React.ReactNode }) {


Look at how these functions are used. Are you sure they take in any ReactNode or are they called with one specific type of node

sheenarbw

You have some guesswork in here. Specifically, your use of GetServerSidePropsContext is dodgy.

In the face of ambiguity, refuse the temptation to guess

If you put guesses in your code then it will be hard for me to trust your code. Every time I've put in the up-front effort to understand things properly it's worked out well for me. Remember that part of the reason you are doing this work is so that you can level up and help others to level up.

One thing I'm not sure of myself if the difference between JSX.Element, ReactElement and ReactNode. So I'm going to need to research that before reviewing everything properly. If you feel like you made any guesses there then please revise that.

sheenarbw · 2023-05-23T02:56:09Z

frontend-mini-challenge/components/LoggedInPage.tsx

+  query,
+  req,
+}: GetServerSidePropsContext) {
+  const whoAmIResponse = await serverSideWhoAmI({ query, req } as GetServerSidePropsContext);


This seems like a lot of guessing...

As far as I know, GetServerSidePropsContext looks like this:

export interface GetServerSidePropsContext { req: IncomingMessage res: ServerResponse params?: ParsedUrlQuery query: ParsedUrlQuery preview?: boolean previewData?: any }

serverSideWhoAmI returns something completely different to that. Just because it is used in server side things doesn't mean that the type is this random thing.

GetServerSidePropsContext is used like this only:

getServerSideProps = async ({ arguments,you,want }: GetServerSidePropsContext) => { ...

In other words, the arguments sent to a getServerSideProps function will adhere to that interface.

If you look at the return below, you can see these lines:

isLoggedIn: whoAmIResponse.status === 200, loggedInUserData: whoAmIResponse.responseData,

This tells us that the whoAmIResponse at least has a status that can be a number, and responseData. The responseData would be a JSON response. So basically an object.

…ed as a type

sheenarbw

Still some rough edges...

frontend-mini-challenge/components/Alerts.tsx

sheenarbw · 2023-05-23T20:04:50Z

frontend-mini-challenge/components/LoggedInPage.tsx

+  children: React.ReactNode;
+  serverSidePropsCorrectlyCalled: boolean;
+  isLoggedIn: boolean;
+  loggedInUserData: { firstName: string; email: string }


This gets copy-pasted all over the place now:

{ firstName: string; email: string }

What if we decide to add a lastName or a cellphoneNumber? Then we'll have to change the code in a million places.

Make an interface in one file, and then import it as needed.

Remember DRY

sheenarbw · 2023-05-23T20:11:41Z

frontend-mini-challenge/components/LoggedInPage.tsx

+  children,
+}: {
+  handleLogout: () => void;
+  loggedInUserData: { firstName: string; email: string }


sheenarbw · 2023-05-23T20:17:36Z

frontend-mini-challenge/components/LoggedInPage.tsx

-  const whoAmIResponse = await serverSideWhoAmI({ query, req });
+
+
+export const getServerSidePropsForLoggedInPage = async (context: GetServerSidePropsContext) => {


explicit is better than implicit.

Are we really expecting the props to look like this?

It looks like we don't use most of that.

Good code makes its intentions clear. So it's important to be specific about what is required.

We are using Typescript so that when other coders work on this project they don't mess up and pass the wrong thing into a function. So we should not accept the wrong thing. We should only accept exactly the correct thing.

sheenarbw

Some repeated comments. Please address everything.

sheenarbw · 2023-05-25T09:00:36Z

frontend-mini-challenge/components/LoggedInPage.tsx

+
+
+
+const LoggedInUserDataType = {


Should this not be an interface?

sheenarbw · 2023-05-25T09:09:35Z

frontend-mini-challenge/components/LoggedInPage.tsx

-  const whoAmIResponse = await serverSideWhoAmI({ query, req });
+
+
+export const getServerSidePropsForLoggedInPage = async ({ query, req }: GetServerSidePropsContext) => {


I have mentioned this one multiple times now. If you do not understand a comment then please do not ignore the comment. I really don't like repeating myself this many times.

Look at how much information is inside GetServerSideProps:
https://github.com/vercel/next.js/blob/v10.2.0/packages/next/types/index.d.ts#L136-L151

if 90% of the interface is completely inaccurate then it's a bad choice.

Please use something appropriate here. GetServerSidePropsContext is inappropriate.

If you disagree then say so. Do not simply ignore this feedback.

sheenarbw

I think you can do better

sheenarbw · 2023-05-29T12:32:49Z

frontend-mini-challenge/components/LoggedInPage.tsx

 import { clearAuthToken } from "../lib/authTokenStorage";
 import { useCookies } from "react-cookie";

+interface Props {


The different components have different props. Please use different interfaces for the different things.

If the foo function has the arguments a, b and c and the bar function has d, e and f then it is incorrect to say that foo requires a,b,c,d,e, f, req and query.

Please make use of the correct interface. It should tell the truth of what is required in each function.. Each function requires different things. We need to be exact and specific. If you add in extra things that aren't needed at all then that makes it worse.

please also make a point of importing things that are defined elsewhere in the code.

and req is definitely not a string.

sheenarbw · 2023-05-29T12:34:23Z

frontend-mini-challenge/components/LoggedInPage.tsx

+    email: string;
+  };
+  handleLogout?: () => void;
+  req?: string;


req isn't used by any function here. Why is it in this interface at all?

sheenarbw

There is still one weird guess in here that I can see.

I need to learn about the different React children prototypes still so there is a chance I've missed something. Please triple-check that there is nothing else that you guessed on. I'm very keen to get this finished.

sheenarbw · 2023-06-01T09:58:30Z

frontend-mini-challenge/components/Alerts.tsx

-export function ErrorAlert({ children, title }) {
+interface Props {
+  title: string;
+  children: string;


it would be better here to say that the children could be a string or another component. Eg if we wanted to have multiple paragraphs of text then a string wouldn't cut it. We would either use multiple Text components or a Stack with a bunch of Text components inside it.

sheenarbw · 2023-06-22T05:08:23Z

frontend-mini-challenge/components/LoggedInPage.tsx

-export async function getServerSidePropsForLoggedInPage({ query, req }) {
-  const whoAmIResponse = await serverSideWhoAmI({ query, req });
+
+export async function getServerSidePropsForLoggedInPage({ query, req }: { query: { [key: string]: string }, req: IncomingMessage }) {


There is no reason to invent things here. Look:

type GetServerSidePropsContext< Q extends ParsedUrlQuery = ParsedUrlQuery > = { req: IncomingMessage & { cookies: NextApiRequestCookies } res: ServerResponse params?: Q query: ParsedUrlQuery preview?: boolean previewData?: PreviewData resolvedUrl: string locale?: string locales?: string[] defaultLocale?: string }

This information comes from https://github.com/vercel/next.js/blob/v10.2.0/packages/next/types/index.d.ts#L136-L151

What datatype is query? What datatype is req? Please use those. We know where these props come from, we know exactly what the types are.

sheenarbw

There is still one thing that looks inaccurate as far as I can see.

Also, this branch is not up to date with the latest code.

Please do the following:

fix the last inaccuracy
merge the latest develop branch into your branch and fix any merge conflicts that come up
make sure that the storybook tests still pass
make sure you can build the application. You can see how to run it by looking at package.json

sheenarbw · 2023-06-27T05:52:39Z

frontend-mini-challenge/components/LoggedInPage.tsx

+  req,
+}: {
+  query: ParsedUrlQuery;
+  req: IncomingMessage;


Please use exactly this for req's type. There is no need to be creative here, we can just copy.

Is there a reason you want to leave out the part about the cookies?

req: IncomingMessage & { cookies: NextApiRequestCookies }

The components folder has been converted to TypeScript.

cec2293

sheenarbw requested changes May 15, 2023

View reviewed changes

storybook tests passing and used expected type instead of any

bf2bb45

sheenarbw requested changes May 23, 2023

View reviewed changes

getServerSidePropsForLoggedInPage function fixed & React.ReactNode us…

67e60ea

…ed as a type

sheenarbw requested changes May 23, 2023

View reviewed changes

Themba Ntuli added 3 commits May 25, 2023 10:13

icon restored and types made dry

6ed2210

fixed merge conflicts

a4cac3b

merge conflict fix

31fa38b

sheenarbw requested changes May 25, 2023

View reviewed changes

removed GetServerSidePropsContext

6b31712

sheenarbw requested changes May 29, 2023

View reviewed changes

made requested changes

b33c697

sheenarbw requested changes Jun 22, 2023

View reviewed changes

fixed alerts props and getServerSide types

e42733c

sheenarbw requested changes Jun 27, 2023

View reviewed changes

Themba Ntuli added 2 commits July 2, 2023 20:42

added cookies NextApiRequestCookies type

228503d

merge conflict fix

a5e0b1c

		const whoAmIResponse = await serverSideWhoAmI({ query, req });


		export const getServerSidePropsForLoggedInPage = async (context: GetServerSidePropsContext) => {

The components folder has been converted to TypeScript. #650

Are you sure you want to change the base?

The components folder has been converted to TypeScript. #650

Uh oh!

Conversation

TheMaja commented May 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description:

Screenshots/videos

I solemnly swear that:

Uh oh!

sheenarbw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sheenarbw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sheenarbw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sheenarbw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sheenarbw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sheenarbw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sheenarbw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

TheMaja commented May 15, 2023 •

edited

Loading