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

Migrating codebase from javascript to typescript #3004

Open
Swarnendu0123 opened this issue Feb 8, 2024 · 17 comments
Open

Migrating codebase from javascript to typescript #3004

Swarnendu0123 opened this issue Feb 8, 2024 · 17 comments
Labels
Area: Documentation For developer documentation related to the p5.js web editor Enhancement Status: Needs Discussion

Comments

@Swarnendu0123
Copy link
Contributor

Swarnendu0123 commented Feb 8, 2024

Increasing Access

By migrating the codebase to TypeScript, we aim to increase accessibility to developers who are familiar with statically typed languages and who prefer the robust type checking provided by TypeScript. This change will facilitate a smoother onboarding process for new contributors and help improve collaboration within the open-source community.

Feature enhancement details

TypeScript offers static type checking, which helps catch potential errors during development, leading to more reliable and maintainable code.

TypeScript provides enhanced IDE support with features like code completion, refactoring tools, and better documentation integration, improving the development experience.

With TypeScript, navigating through the codebase becomes more efficient due to its support for defining explicit types and interfaces, enabling developers to quickly understand the structure of the application.

But, first we have to define the steps to migrate to Typescript.

@rahulrana701
Copy link
Contributor

Migrating only client side part to typescript is not a feasible approach in my opinion , as we are building the whole project
together via webpack and if we convert only the client side into typescript we may get a lot of errors on the go , so it is better to migrate the whole project from Javascript to typescript but in this approach of migrating the whole project , we have to make some changes in the webpack files so that it could convert the ts files into js files and we have to make lot of other changes in the backend code and client code as well , ideal and best approach is to use monorepos or turborepos but in this approach we may have to bootstrap the project from start in some cases , @lindapaiste @raclim what are your views on this , if you want to convert this project from javascript to typescript I can give it a try you , I will open a new issue for that and you could assign me that and after that I will start working on it.

@Swarnendu0123
Copy link
Contributor Author

Well I think, this is not a issue which can be solved by a single person, the codebase is so large, generally when we convert any project from .js to .ts for type safety and for less errors in long run.

Your enthusiasm is quite appreciated @rahulrana701, but as I opened this issue, let me work on this, if at any point of time I need your help, definitely I will ask you.

@raclim @lindapaiste can you assign this issue to me, I will not delete the .js or .jsx files, instead of that I will try do go with the basic configuration and convert the client side code to typescript. after that step by step, We can convert the whole codebase to typescript.

@rahulrana701
Copy link
Contributor

@Swarnendu0123, you can absolutely work on this issue if you want, brother. It's just that you didn't mention it initially, so I thought you didn't want to work on it. But it's okay. In my opinion, it's not so large that we need more than one developer to convert it to TypeScript; it can be done alone. However, you can continue

@Swarnendu0123
Copy link
Contributor Author

@Swarnendu0123, you can absolutely work on this issue if you want, brother. It's just that you didn't mention it initially, so I thought you didn't want to work on it. But it's okay. In my opinion, it's not so large that we need more than one developer to convert it to TypeScript; it can be done alone. However, you can continue

Thanks, actually I am just waiting for @raclim and @lindapaiste's approval and views on the issue. And any method they can suggest.

@rahulrana701
Copy link
Contributor

@Swarnendu0123, you can definitely reach out to me if you need any help because I know how to convert this whole project into TypeScript.

@raclim
Copy link
Collaborator

raclim commented Feb 8, 2024

Thanks for raising this!

To be honest, I don't feel comfortable with implementing a change of this scale (which also extends to implementing this to the entire codebase proposed in #3007). Although TypeScript has become much more widely used and offers more capabilities and robustness, it adds complexity, a much higher learning curve, and requires a lot of dedicated time to implement, which I'm not sure is worth its benefits. In comparison, I feel that JavaScript is easier for newer contributors/developers to learn and has more resources for support/learning due to its longevity, which aligns with p5.js's values for access and beginner-friendly initiatives.

Overall, I don't feel a strong need to change the entire language we're using if what we have already is working relatively well and is still widely used, or at least jumping into this type of change any time soon. However, I probably am biased towards Javascript in general and don't use TypeScript as often. @lindapaiste would love to here your thoughts here as well!

@Swarnendu0123
Copy link
Contributor Author

Swarnendu0123 commented Feb 8, 2024

Javascript is a very good language and it is also a very good language for beginners, I completely agree with you @raclim. but during open source contributors many open source contributors face issues while developing, and are not aware of type safety. For the long run, contributors also can gain compatibility in typescript.

It is a very big change, I completely agree. But what we can do is, we can start developing typescript files by small steps. We can start converting the components first, then the other client side logics, state management logics and the whole client side codebase.

And when it is well tested completely and well implemented, we can migrate to Typescript. Till then we can have the typescript files in the same folder but we will not use it in production.

If you give me approval, I can start working on this. Also I have mentioned in this issue.

What is your opinion @lindapaiste ?

@raclim
Copy link
Collaborator

raclim commented Feb 8, 2024

Just to add, I'm probably going to get a few more opinions on this as well, both within and outside of the online realm!

@rahulrana701
Copy link
Contributor

rahulrana701 commented Feb 8, 2024

@lindapaiste @raclim as I have mentioned earlier if we need to use typescript in a project , best approach is to setup a turbo repo but in this case we can't we , if yes , we have to bootstrap the project from start which is not appropriate , and if we migrate this to typescript it will create a mess and we have to do look on to each and every line of code properly. So in my opinion we are good to go with javascript.

But we can still convert it into ts but it is gonna be really messy.

@lindapaiste
Copy link
Collaborator

I will chime in with more thoughts later but I don’t think that the build process is as complicated as what you are describing. You can have a project with a mix of TS and JS so it doesn’t need to be done all at once either. Look through the open PRs on the ml5.js repo and find a PR from me with the changes to webpack to support a mixed TS and JS codebase. It could be a little different here but that can be a starting point or at least a reference.

@rahulrana701
Copy link
Contributor

@lindapaiste yes it can be converted just have to make some changes in webpack files and convert js files to ts files and make some changes by viewing every file necessary

@Swarnendu0123
Copy link
Contributor Author

Swarnendu0123 commented Feb 8, 2024

I will chime in with more thoughts later but I don’t think that the build process is as complicated as what you are describing. You can have a project with a mix of TS and JS so it doesn’t need to be done all at once either. Look through the open PRs on the ml5.js repo and find a PR from me with the changes to webpack to support a mixed TS and JS codebase. It could be a little different here but that can be a starting point or at least a reference.

This can be the starting point, @lindapaiste can you suggest fron where to start?

@lindapaiste
Copy link
Collaborator

This is the PR that I was mentioning: ml5js/ml5-library#1388

The minimum that is required for the setup is:

  • Create a tsconfig.json with “allowJs”: true
  • Configure Webpack to use ts-loader to load .ts or .tsx files
  • Configure eslint to parse .ts or .tsx files with @typescipt-eslint

@rahulrana701
Copy link
Contributor

rahulrana701 commented Feb 9, 2024

@Swarnendu0123 are you working on this ??

@Swarnendu0123
Copy link
Contributor Author

Swarnendu0123 commented Feb 9, 2024

@Swarnendu0123 are you working on this ??

Yes, it will take time as I already mentioned, I have to check every aspect of it before raising any PR.

@lindapaiste
Copy link
Collaborator

lindapaiste commented Feb 12, 2024

Okay so there's lots to discuss here. Before we get bogged down in implementation details I think it's important to come to a consensus on whether we think that using TypeScript is an improvement.

Pros:

  • Minimizes the risk of run-time errors because we are aware of potential problems at compile time.
  • Keeps up with current best-practices, as the React community in general is moving away from Prop-Types and towards TypeScript.
  • Intellisense in VSCode (and other editors) will be able to suggest property names on objects like event, project, event.target, etc. if those objects have the correct types.
  • Contributors will have a better idea of what each variable represents when they are looking at a piece of code. They will immediately see if they are accessing a property which doesn't exist, and they can jump to the source definition of the object to see all of the properties.
  • It allows us to define specific types for Redux actions and make sure that we are dispatching the right payload.
  • I personally have a strong preference for writing TypeScript over JavaScript.
  • I'm extremely good at using TS in React and I am able to fix issues in PRs.

Cons:

  • Most coders learn pure-JS first, so we are increasing the amount of knowledge that it takes to contribute to the codebase.
  • TypeScript errors can sometimes be really long-winded and hard to understand. The most frustrating ones are related to HTML elements, like ref={inputRef} or changing the element of a styled-component with as="div".
  • Users don't always know the right ways to address TS issues and can end up with a mess of down-stream fixes like as assertions. For example, when the initial state is an empty array or an empty object, we always need to define the type with a generic like useState<string[]>([]). It is not an error to use useState([]) but you will get errors further down in the code where you use the untyped value. Inexperienced coders are often tempted to make changes to the line which has the error message because they do not understand where or what the root cause is. We might be able to avoid some of this with eslint rules.
  • PropTypes checks the types of the actual objects that are provided at run-time, which verifies that the types you declared are actually correct. TypeScript does not do this.

Neutral:

  • We are already using PropTypes on all of our components.
  • We could get some of the benefits of TS by using more/better JSDoc comments.

@Swarnendu0123 We do not need to (or want to) convert everything at once.

IF we decide that this is something which we want to do, which we have not decided yet, the steps would be:

  1. Create a PR which enables using TypeScript, but only rewrites one single file to test the system. As I explained in this comment. We would start with a very permissive TS configuration with "strict": false and "allowJs": true.
  2. Automatically migrate our existing component PropTypes to TS using a tool like Ratchet and create a PR with those results.
  3. Create additional PRs to add type definitions to local variables, functions defined within components (handleClick, etc.), utility functions, API calls, etc.
  4. Add stricter rules to our TS configuration such as "noImplicitAny": true and fix the violations that pop up with those rules. Possibly one PR per rule. Possibly we focus on step 3 and only add additional rules once we know that we're no longer violating it.

As for the question of client vs. server, I don't see any arguments being made here as to why we should address the client portion first. I would argue the opposite, that we should handle the server first, for the following reasons:

  • The client side relies on objects which are defined on the server side, such as project, collection, user, etc., but not vice-versa. Once we define those types and interfaces on the server we can import into the client code.
  • The client side already has some measure of type-safety due to PropTypes but the server has none.
  • Server-side code is (subjectively) already more complex and less beginner friendly, so the increased learning curve is less of a concern.

I would put the order of addressing things as:

  1. Mongoose code on server
  2. Express code (API handlers) on server
  3. Utility functions on client and server
  4. Redux code on client
  5. Components on client

I could also envision a scenario where we only do steps 1-4 and convert all of the "logic" code to TS but keep the components as pure JS for the sake of beginner-friendliness.

@Swarnendu0123
Copy link
Contributor Author

Swarnendu0123 commented Feb 12, 2024

Cool! thanks for your guidance @lindapaiste, you suggested to raise a PR with the basic configuration and maybe one or two files changed to ts. And also I completely agree with you that the server side code has till now no type safety, so we should target the server side first. I will raise with the basic config on this issue.

@lindapaiste lindapaiste added Area: Documentation For developer documentation related to the p5.js web editor Status: Needs Discussion labels Feb 13, 2024
@lindapaiste lindapaiste changed the title Migrating client side codebase from javascript to typescript Migrating codebase from javascript to typescript Feb 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Documentation For developer documentation related to the p5.js web editor Enhancement Status: Needs Discussion
Projects
None yet
Development

No branches or pull requests

4 participants