-
Notifications
You must be signed in to change notification settings - Fork 134
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
Create type definitions for expensify-common #553
Create type definitions for expensify-common #553
Conversation
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
I have read the CLA Document and I hereby sign the CLA |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One typo otherwise looks good to me
also, we have is this understanding correct, @blazejkustra ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good so far, it's easier to read this code now. I'll continue my review another day.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 🙇 🙇 🙇
@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 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 |
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
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? |
The approach to write type declarations is only a temporary solution,
Yes this brings more work, but it is a temporary solution and eventually declaration files will be replaced with TS implementations. These declaration files (
Even with
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. |
@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
I agree with @blazejkustra on this. There is no option to start migrating NewDot without doing anything to So there are only two options:
An important point to keep in mind is that type declaration files don't compromise type safety. What do you think @roryabraham ❓ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 🙌
That's exactly what I did, maybe not the fastest way but it works 😄 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice work 🎉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
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. |
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
QA
N/A