Migrating to Typescript

treasure-data / digdag

Workload Automation System

https://www.digdag.io/

Apache License 2.0

1.3k stars 221 forks source link

Migrating to Typescript #1665

Open szyn opened 2 years ago

szyn commented 2 years ago

seiyab commented 2 years ago

Can I work on it?

szyn commented 2 years ago

Sure, no problem at all. I appreciate your help as usual 👍

seiyab commented 2 years ago

@szyn Which plan is better do you think?

Gradually migrate
- steps
  1. Make all the codes valid typescript syntax without resolving type errors and lint errors
  2. Gradually collect type errors and lint errors by many small PRs.
  3. When all the type errors and lint errors are resolved, add type check step and lint step into CI.
- pros
  - can avoid conflicts
  - each PRs will be small and easier to review
  - other contributors can help migration
  - some progress will be commited even if I stop commit for some reason
Migrate in single PR
- steps
  1. Make syntax, type, format and coding style valid at one PR and check them on CI
- pros
  - can avoid committing half done codes
  - you will review PR only 1 time
  - can avoid half done migration even if I stop migration for some reason

szyn commented 2 years ago

Thank you, I feel that 2 is simple and better for me. What do you think?

seiyab commented 2 years ago

I think each of them is good. I'll try latter one.

seiyab commented 2 years ago

@szyn I found that fixing all the typing error in reliable way is hard. It is similar problem to newer flow that reports too many type errors. Many type annotations are unclear.

So I will add some any, that is unclear type and some ts-ignore, that make type checker skip codes. Do you agree it?

This approach is based on original approach 2, but it also has some flavor of original approach 1.

No type error will be reported by type checker after the single PR.
So we can run type checker on CI.
However, a part of type annotations (any) don't make sense, and even be ignored sometimes.
- So, After migration, I recommend that request change for PRs that adds more any or ts-ignore to make type annotations more reilable.
- I'm going to additional PRs that will resolve some part of anys and ts-ignores after migration.

szyn commented 2 years ago

So I will add some any, that is unclear type and some ts-ignore, that make type checker skip codes. Do you agree it?

Yes, I agree with it. And also it looks good to me for the approach you suggested. Thank you for letting me know 👍