Skip to content
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.

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

Add TypeScript #174

Open
vmcodes opened this issue Jun 12, 2023 · 9 comments
Open

Add TypeScript #174

vmcodes opened this issue Jun 12, 2023 · 9 comments
Assignees
Labels
question Further information is requested team-discussion

Comments

@vmcodes
Copy link
Collaborator

vmcodes commented Jun 12, 2023

What do we need to build or fix?
We'd like to transition the site to TypeScript. This will likely include creating global types to be used, as well as creating standards for how to define props passed between components.

Technical details

  • Create a new branch named chore/add-typescript and checkout to it.
  • Create a types folder to keep global types and interfaces easily accessible.
  • Define standards for passing in props and where types should be used in general.
  • Determine if we should define functional components using the FC import from React, or if we should keep the current components as is and define the types inline.
  • Test the feature in different screen sizes and browsers to ensure it looks and functions as expected.
  • Commit the changes to the chore/add-typescript branch and create a pull request to merge the changes into the main branch.
  • Update the CHANGELOG.md file.

Approach suggestions
There are a variety of valid approaches. We should probably discuss here in a thread what would be the best fit for Web Dev Path. Any thoughts or opinions should be included below, and would be appreciated.

Deadline
Please remember that once you assign this task to yourself, you'll need to complete it in 3 weeks.

Acceptance criteria

  • Test the section and components in many screen sizes, you can use the Inspect tool for that.
  • Please test if the new changes added to the components do not affect the other instances.
  • Test the feature in many browsers, such as Chrome, Firefox, Edge, and Safari (MAC).
  • If there are any build problems when submitting your PR, run yarn build locally to solve the issues and commit the changes.
  • Update the CHANGELOG.md file.
@mariana-caldas mariana-caldas added question Further information is requested team-discussion labels Jun 13, 2023
@mariana-caldas
Copy link
Member

Thanks for creating this issue, @vmcodes !

While we are in the process of implementing a theme on issue #165 (or even before we start it), let's focus on this issue which aims to convert the project's js files to TypeScript (ts). This conversion will involve creating interfaces and global types for each component, making the project more resilient (as mentioned in this article: https://blog.bitsrc.io/5-strong-reasons-to-use-typescript-with-react-bc987da5d907) and better equipped for debugging and future testing.

In addition to determining who will work on this issue, it's crucial to decide on the approach. Should we assign it to a single developer, as we did with the styled-components implementation #162, or should we break it down into smaller issues to convert the project modularly? I'd appreciate hearing your thoughts on this matter @Web-Dev-Path/development-team

@cherylli
Copy link
Member

In my opinion, we can break it down as there's not much variations in styles for typescript, unlike styled components

@vmcodes
Copy link
Collaborator Author

vmcodes commented Jun 13, 2023

I'm open to taking on the task alone, but if there's someone who wants to gain some TypeScript experience, I don't mind working with anyone.

As far as approach, I think that we shouldn't modify the component structure using something like:

const MyFunction : FC<{MyProps}> = ({ props }) =>

I think it'll just add confusion to an already solid foundation. We could do something more along the lines of:

export default function MyFunction({ props } : MyProps)

A question I had was, how strict do we want to be with types? Should we define all variables and avoid leaving anything undefined or as type any unless completely necessary?

Last question from me would be, do we want to keep all types in one file or define prop types within the component file if they aren't reused?

@cherylli
Copy link
Member

cherylli commented Jun 13, 2023

even just const Component = ({prop}: PropsType) => {...} this is what i've been using

We should be strict otherwise there's no point using typescript. Only place we should use any is when we don't know what type we are getting e.g. like a generic API response but I don't think we will have that issue

@mariana-caldas
Copy link
Member

Hey, team!
I've just converted to TypeScript this small React project and I think we can use this approach as a reference:
https://github.com/mariana-caldas/react-interview-prep-playground

If we decide to tackle it with just one dev, I'd recommend starting converting the components first (not the pages).

@cherylli
Copy link
Member

Hey, team! I've just converted to TypeScript this small React project and I think we can use this approach as a reference: https://github.com/mariana-caldas/react-interview-prep-playground

If we decide to tackle it with just one dev, I'd recommend starting converting the components first (not the pages).

do we have to use React.FC like in your test project? Actually never used it and I did some research I think we should stay away from it

facebook/create-react-app#8177
https://medium.com/@harrymt/should-you-use-react-fc-for-typing-react-components-62cde9ba67c#

@mariana-caldas
Copy link
Member

Hey, team! I've just converted to TypeScript this small React project and I think we can use this approach as a reference: https://github.com/mariana-caldas/react-interview-prep-playground
If we decide to tackle it with just one dev, I'd recommend starting converting the components first (not the pages).

do we have to use React.FC like in your test project? Actually never used it and I did some research I think we should stay away from it

facebook/create-react-app#8177 https://medium.com/@harrymt/should-you-use-react-fc-for-typing-react-components-62cde9ba67c#

Hey @cherylli and @vmcodes ,

By reading the article, it seems it is a matter of preference, however, because we're converting the project into TS (not creating it from scratch) it may be useful to stick with the traditional approach since it may help us to not miss any type annotation. So, I'd say, let's use the more beginner-friendly approach as per the reference.

@vmcodes
Copy link
Collaborator Author

vmcodes commented Jun 14, 2023

@mariana-caldas I'm cool with that. Were you able to view the draft PR I made? I can resolve the build issues if you think it would be good to at least change the file name extensions for now. You can write JS in TypeScript files and I thought it might be a good idea incase someone wants to start working on the theming issue at the same time. It would avoid a lot of merge conflicts.

@mariana-caldas
Copy link
Member

mariana-caldas commented Jun 16, 2023

I wanted to share this insightful post by @cherylli on our Slack channel, where she explains why we should move away from using the React.FC approach in our TypeScript conversion.

image

In this React repository pull request, you can find a detailed explanation about why Facebook decided to drop React.FC from their projects. The primary reason is that React.FC introduces some limitations with TypeScript, which includes the following points:

  1. Implicit Children: React.FC automatically injects children into your component props, even if you don't want or need them. This can be problematic when you're trying to strictly define the props your component should accept, and children isn't one of them.

  2. Non-Standard Function Signatures: When using React.FC, the function signature becomes non-standard. A regular function allows for a variety of parameter types, whereas React.FC specifically requires an object.

  3. Generics Are More Difficult: React.FC can make it harder to correctly type generic props.

  4. Defaults and Required Flags: React.FC doesn't play nicely with defaultProps and the required flag in the context of TypeScript.

Considering these points, we can type our functional components like this:

type MyComponentProps = {
  name: string;
  age: number;
};

const MyComponent = ({ name, age }: MyComponentProps) => {
  // ...
}

Over the next few days, I'll be updating the project references to align with this approach. Thanks so much for highlighting this issue, Cheryl!

Please let me know your thoughts on this, team!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested team-discussion
Projects
None yet
Development

No branches or pull requests

3 participants