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

[TS migration] Implement useOnyx hook in react-native-onyx #34339

Closed
blazejkustra opened this issue Jan 11, 2024 · 42 comments
Closed

[TS migration] Implement useOnyx hook in react-native-onyx #34339

blazejkustra opened this issue Jan 11, 2024 · 42 comments
Assignees
Labels
NewFeature Something to build that is a new item. Weekly KSv2

Comments

@blazejkustra
Copy link
Contributor

Typescript migration (react-native-onyx)

Make sure you read through our TypeScript's style guide, cheatsheet and PropTypes conversion table before you start working on this migration issue.

Files

N/A

@blazejkustra blazejkustra added Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 labels Jan 11, 2024
Copy link

melvin-bot bot commented Jan 11, 2024

Triggered auto assignment to @JmillsExpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

@mountiny mountiny removed the Bug Something is broken. Auto assigns a BugZero manager. label Jan 11, 2024
@mountiny
Copy link
Contributor

Will be handled by agencies

@roryabraham roryabraham self-assigned this Jan 12, 2024
@roryabraham roryabraham added Weekly KSv2 and removed Daily KSv2 labels Jan 12, 2024
@roryabraham
Copy link
Contributor

Co-assigning to handle reviews

@roryabraham roryabraham changed the title [TS migration] Implement useOnyx hook in react-native-onyx [HOLD][TS migration] Implement useOnyx hook in react-native-onyx Jan 12, 2024
@roryabraham roryabraham added the NewFeature Something to build that is a new item. label Jan 12, 2024
Copy link

melvin-bot bot commented Jan 12, 2024

@roryabraham
Copy link
Contributor

Putting on HOLD until it's picked up by an agency assignee

@roryabraham
Copy link
Contributor

@melvin-bot melvin-bot bot removed the Overdue label Jan 22, 2024
@blazejkustra
Copy link
Contributor Author

Hey! I’m Błażej Kustra from Software Mansion, an expert agency, and I’d like to work on this issue together with @fabioh8010

@fabioh8010
Copy link
Contributor

Commenting for assignment too.

@roryabraham roryabraham changed the title [HOLD][TS migration] Implement useOnyx hook in react-native-onyx [TS migration] Implement useOnyx hook in react-native-onyx Jan 24, 2024
@fabioh8010
Copy link
Contributor

fabioh8010 commented Jan 25, 2024

Update: Still working on the doc, and started doing some tests with initial implementation.

@fabioh8010
Copy link
Contributor

Update: useOnyx proposal doc created and sent to review!

Draft PR of POC implementation also created, but it's still experimental and WIP.

@roryabraham
Copy link
Contributor

Link to the proposal in #expensify-open-source is here.

We've got a clear consensus to move forward, so the next step is to complete the implementation of the hook and open up the PR for reviews. I look forward to reviewing it.

@fabioh8010 @blazejkustra when do you estimate that will be ready?

@fabioh8010
Copy link
Contributor

@roryabraham @blazejkustra Created a Draft PR and left an update, let's continue discussions there.

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Weekly KSv2 labels Apr 4, 2024
@fabioh8010
Copy link
Contributor

PR is ready to review @roryabraham

@fabioh8010
Copy link
Contributor

Update: PR still under review.

@ishpaul777
Copy link
Contributor

Hey 👋 @roryabraham , Can you please assign me here

@roryabraham
Copy link
Contributor

roryabraham commented Apr 15, 2024

coming from https://expensify.slack.com/archives/C0593T50UHG/p1712936660132249, one more follow-up task ... let's return undefined instead of null for missing Onyx values

@roryabraham roryabraham removed the Reviewing Has a PR in review label Apr 15, 2024
@roryabraham
Copy link
Contributor

Also, @CortneyOfstad can we please issue a $250 payment to @ishpaul777 for their review of #38924? Thanks!

@CortneyOfstad
Copy link
Contributor

Sorry for the delay here! @ishpaul777 — I sent you a manual offer in Upwork here. Please let me know once you accept and I can get that paid ASAP. Thanks!

@ishpaul777
Copy link
Contributor

Thanks Accepted offer!

@fabioh8010
Copy link
Contributor

coming from https://expensify.slack.com/archives/C0593T50UHG/p1712936660132249, one more follow-up task ... let's return undefined instead of null for missing Onyx values

Update: PR is up! cc @roryabraham @blazejkustra

@CortneyOfstad
Copy link
Contributor

Thanks Accepted offer!

Payment completed @ishpaul777 — thank you!

@CortneyOfstad
Copy link
Contributor

@roryabraham is anything else needed here, or can this be closed?

@fabioh8010
Copy link
Contributor

coming from https://expensify.slack.com/archives/C0593T50UHG/p1712936660132249, one more follow-up task ... let's return undefined instead of null for missing Onyx values

Update: PR is up! cc @roryabraham @blazejkustra

@CortneyOfstad We are still working on this issue 🙂

@fabioh8010
Copy link
Contributor

Update: I've opened the PR to review, but I put on HOLD because I'm going to be OOO next week, and will return on May 6th.

@CortneyOfstad
Copy link
Contributor

Sounds good — thanks @fabioh8010!

@fabioh8010
Copy link
Contributor

Update: I've took the PR off the hold state since I'm back now, so we can resume review.

@CortneyOfstad
Copy link
Contributor

Waiting for the merge freeze to be lifted 👍

@CortneyOfstad
Copy link
Contributor

Excuse my ignorance, but I see the PR is listed as "Published to npm in v2.0.37". Does that mean it is completed? I traditionally see it go through the flow of stating to production, but with it being Onyx, I just wanted to make sure I had this correct. Thanks! CC @roryabraham

@fabioh8010
Copy link
Contributor

fabioh8010 commented May 15, 2024

I think we can close this issue - The hook is implemented and already being used in E/App, and the last type improvement is merged and being handled in this Onyx bump PR, so I guess there is nothing left to do here. But I will leave to @roryabraham too to decide 😅

@CortneyOfstad
Copy link
Contributor

Going to close — feel free to reopen @roryabraham if needed 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NewFeature Something to build that is a new item. Weekly KSv2
Projects
Development

No branches or pull requests

7 participants