-
Notifications
You must be signed in to change notification settings - Fork 136
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
[TS migration] Setup typescript in expensify-common & Migrate CONST, fastMerge, Device, Logger to Typescript #699
[TS migration] Setup typescript in expensify-common & Migrate CONST, fastMerge, Device, Logger to Typescript #699
Conversation
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
@tgolen I thought I'd bring you in to review this one, since I may not be the best suited to confirm that everything is as it should... I haven't seen anything wrong in the code, but I think a second more veteran opinion is worth it here. |
I'm really sorry. I was not able to review this today due to being heads-down getting comp review ready. I will be OOO for the next 1.5 week, so I will assign someone else to review. |
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.
Also, what is the plan for publishing this? Right now, expensify-common is consumed via a git hash in E/App's package.json
That could work if we commit dist
, but I see you've addd it to the .gitignore
. So what's the plan? If we need to publish this to npm can you copy the workflow from react-native-onyx?
I think copying the workflow from |
@roryabraham I copied over the
FYI: There is If you think this PR is too big, I can move publish workflow and related changes to a separate PR. |
Also, please have another look at the code, I changed it slightly! 🚀 |
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 but let's get those secrets added before we merge
Created internal ticket to add those GitHub secrets |
Let's see if publishing works as expected... https://github.com/Expensify/expensify-common/actions/runs/9261388961 |
🚀Published to npm in v2.0.1 |
Looks like that worked (after a few more minor adjustments) 🎉 |
Awesome! Thanks @roryabraham for the help on this one 🙌 |
Going forward expensify-common code can be written in Typescript, we have to take into consideration transpiling step now.
TS code will live in
lib
catalog and transpiled code will end up indist
catalog. Thelib
catalog won’t be published to npm; instead, we’ll only publish thedist
catalog.expensify-common
imports will look like this:Here is a similar PR that prepared Onyx for a similar migration.
Fixed Issues
$ Expensify/App#42245