Create type definitions for expensify-common

[blazejkustra]

This is part of Expensify/App#16665 issues. The plan is to write type definitions only for the files that are used in App.

Fixed Issues

$ #547

Tests

• - Check that type declarations (.d.ts files) are correctly typed and in sync with Javascript implementations.
• - Check that types work as expected in the App

QA

N/A

[github-actions]

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

[blazejkustra]

I have read the CLA Document and I hereby sign the CLA

[mountiny]

One typo otherwise looks good to me

[hayata-suenaga]

expensify-common doesn't have index.js that exports everything? (or do we have a script that generates index.js???) I was wondering that we might need index.d.ts from which we export all types (i.e. types used in App). but I guess if the library itself doesn't have index.js file, then we don't need to do this?

also, we have index.js specified as the main file in package.json. I believe libraries also usually have to specify types or typings field for the main d.ts file that is used when the main file is imported. But as we actually don't have index.js file, we don't have to specify types field?

is this understanding correct, @blazejkustra ?

[neil-marcellini]

Looking good so far, it's easier to read this code now. I'll continue my review another day.

[hayata-suenaga]

I think I commented this without a link to the code... here is the relevant code portion:

// Url.d.ts
declare const TLD_REGEX: "XN--VERMGENSBERATUNG-PWB|XN--VERMGENSBERATER-CTB|XN--

@blazejkustra I'm so sorry. I think my comment was misleading.

I suggested declaring TDL_REGEX in Url.d.ts file to be used inside string interpolations for other constants.

But we decided not to do that

There is no usage for TLD_REGEX as it's not exported from this file.

Also, for the type for tlds.js to be used when tlds is imported, we need to create a separate file tlds.d.ts

so your original code that puts declaration for tlds inside tlds.d.ts was better

could you revert to that again I'm sorry for the confusion 🙇 🙇 🙇

[hayata-suenaga]

@blazejkustra thank you for the great job and quick fix 🙌

Just a small adjustment left: I realized that types are still left in JSDoc comments in CONST.d.ts

Is it possible to remove them? 🙇

also @mountiny suggested here to add a new line at the end of each file. Unless it's a conversion not to include a new line in d.ts files (I'm suspecting it might because tsc didn't add new lines?), let's add new lines for the sake of consistency with other files 👍

[roryabraham]

This PR seems pretty crazy... I was on-board with approach only as a temporary measure, but seeing the actual PR is quickly dissuading me again

1. It seems like creating and maintaining this definition file is as much or more work than just re-writing this code using TypeScript and publishing the package as JS
2. It's also much less valuable since we have skipLibCheck set to true in our E/App tsconfig, so errors in these type declaration files will be ignored.

What would be the consequence if we just didn't do this at all right now, then properly implemented TypeScript in expensify-common at a slower pace?

[blazejkustra]

This PR seems pretty crazy... I was on-board with approach only as a temporary measure, but seeing the actual PR is quickly dissuading me again

The approach to write type declarations is only a temporary solution, expensify-common will be migrated to Typescript right after App migration is finished.

It seems like creating and maintaining this definition file is as much or more work than just re-writing this code using TypeScript and publishing the package as JS

Yes this brings more work, but it is a temporary solution and eventually declaration files will be replaced with TS implementations.

These declaration files (.d.ts) were generated with tsc - meaning that I have .ts files which are almost ready (there are some TS warning/errors that would have to be fixed). The problem is that expensify-common is quite outdated and Typescript setup in this repo would require much more changes like removal of grunt, babel, CI/CD changes and more. We agreed that E/App is the priority and type declarations in this repo will be eventually replaced with Typescript implementations. That's why I believe it is better to just stick to type declarations for now.

It's also much less valuable since we have skipLibCheck set to true in our E/App tsconfig, so errors in these type declaration files will be ignored.

Even with skipLibCheck: false Typescript doesn't check type declaration files with it's Javascript implementations. This flag is more about conflicts between different packages.

What would be the consequence if we just didn't do this at all right now, then properly implemented TypeScript in expensify-common at a slower pace?

A lot of @ts-ignore/@ts-expect-error annotations in E/app, and possibly some runtime errors that comes from wrong function usage. Type declarations are much better than nothing in my opinion.

cc @hayata-suenaga @roryabraham

[hayata-suenaga]

@roryabraham thank you for sharing your concern, and I apologize if we haven't communicated to you well the reasoning behind why we agreed upon creating declaration files for expensify-common.

Type declarations are much better than nothing in my opinion.

I agree with @blazejkustra on this. There is no option to start migrating NewDot without doing anything to expensify-common. Anything is better than no type information.

So there are only two options:

1. Halt NewDot migration until we perfect typings of expensify-common and onyx
2. Create type declaration files for exposed functions/methods and push forward NewDot TS migration.

An important point to keep in mind is that type declaration files don't compromise type safety.

What do you think @roryabraham ❓

[hayata-suenaga]

didn't realize there were type annotations left in JSDoc in sr.d.ts @blazejkustra thank you for removing them 🙇

The code looks 👍 to me now.

@blazejkustra So you copied contents of .js files to .ts files and added types so that tsc can create d.ts files? Nice idea and thank you for your work 🙌

[blazejkustra]

@blazejkustra So you copied contents of .js files to .ts files and added types so that tsc can create d.ts files? Nice idea and thank you for your work 🙌

That's exactly what I did, maybe not the fastest way but it works 😄

[neil-marcellini]

Looking great. I think we should remove the @param annotations completely and then I'm happy with it. We still have them in JS if someone wants to read a description of the parameters.

[hayata-suenaga]

nice work 🎉

[fabioh8010]

LGTM!

[neil-marcellini]

I'm going to merge this because 4 of us agree it's good to go and I want to keep things moving. We can always do minor fixes in follow ups if needed.