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

Easier way to type parameters and return types of class implementations using types in index.d.ts? #69

Closed
justingrant opened this issue Oct 1, 2021 · 4 comments

Comments

@justingrant
Copy link
Contributor

I learned something sad today: TS doesn't automatically infer the types of methods that implement interface methods. Example:

// index.d.ts
namespace Temporal {
  class Foo {
    bar(s: string): number;
  }
}

// foo.ts
class Foo implements Temporal.Foo {
  bar(s) {
    //^ Parameter 's' implicitly has an 'any' type.(7006) :-(
    console.log(s.toUpperCase());
    return 42;
  }
}

Details:

This means that even though we have detailed types in index.d.ts, if we want to remove noImplicitAny then we'll still need to figure out how to get type annotations to the parameters of every one of hundreds of Temporal class methods.

I think this will have to be done manually by copy/pasting from index.d.ts into each class implementation, but I'm opening this issue in case there's an easier way to do this.

cc @12wrigja

@12wrigja
Copy link
Contributor

12wrigja commented Oct 1, 2021

I was wondering this myself last night.... I think there might be a way to use the TS Compiler APIs to write a program that runs the typechecker and and insert the relevant types where there aren't any by manually checking to see if the class a function is defined in uses some type from the Temporal namespace and copying it over.

Removing noImplicitAny (and turning on all the currently commented out options in the tsconfig) is a hard requirement of mine, so I'll be working on this at some point.

@justingrant
Copy link
Contributor Author

Removing noImplicitAny (and turning on all the currently commented out options in the tsconfig) is a hard requirement of mine, so I'll be working on this at some point.

Yeah, I've been experimenting with this too. It's how I ran into the issue above. I'll push a draft PR sometime in the next few days so you can see what I've found. I'm starting with ecmascript.ts and calendar.ts and moving on to other files if I have time. Want to coordinate on this work? We could plan to meet in the middle like the transcontinental railroad. :-)

I think there might be a way to use the TS Compiler APIs to write a program that runs the typechecker and and insert the relevant types where there aren't any by manually checking to see if the class a function is defined in uses some type from the Temporal namespace and copying it over.

I suspect that it's probably easier to manually copy/paste types than to write one-off code to do it. My guess is that adding parameter and return types to every method is about 20-40 mins per Temporal type. Probably not worth automating. I've started on calendar.ts which is probably the hardest non-ecmascript.ts file to type and I'll probably be done with it in less than an hour.

That said, a PR to Typescript that added a refactoring that could be used in IDEs to do this for any type, or an extension to the "infer from usage" feature of TS, would be amazing!

@12wrigja
Copy link
Contributor

12wrigja commented Oct 2, 2021

Want to coordinate on this work

Definitely. To be clear, are you talking about just copying the types over, or are you also (temporarily) changing compiler flags at the same time and including those changes?

I have a branch started that migrates to JSBI, and part of that has been some whack-a-mole adding types to find all the places where the big-integer impl is used and where values are numbers, big-integer, or something else entirely. Most of this is within either ecmascript or slots, but there's probably some other small adjustments in other files that could be split out.

https://github.com/js-temporal/temporal-polyfill/compare/main...12wrigja:jsbi?expand=1

calendar.ts
debug.ts
duration.ts
ecmascript.ts
index.ts
init.ts
instant.ts
intl.ts
intrinsicclass.ts
legacydate.ts
now.ts
plaindatetime.ts
plaindate.ts
plainmonthday.ts
plaintime.ts
plainyearmonth.ts
regex.ts
slots.ts
temporal.ts
timezone.ts
zoneddatetime.ts

seems to be the entire list, and some files are trivial. Why don't you work top-down (as you've already started on Calendar), and I'll work bottom-up? now.ts seems to be the meeting point.

@justingrant
Copy link
Contributor Author

Looks like things have moved on since I opened this issue. Now AFAIK only part of calendar.ts needs better typing; all the other files should be already converted. So we can probably close this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants