search

Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.46k stars 2.82k forks source link

Migrate MapView to useOnyx #50343

Open neil-marcellini opened 4 days ago

neil-marcellini commented 4 days ago

Problem

Coming from this PR ES Lint changed files check is failing because we need to migrate to useOnyx

https://github.com/Expensify/App/pull/41242#issuecomment-2391981853

Solution

Do the migration

Issue OwnerCurrent Issue Owner: @janicduplessis
melvin-bot[bot] commented 4 days ago

Triggered auto assignment to @twisterdotcom (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

neil-marcellini commented 4 days ago

@janicduplessis is going to take this one since he touched the code that prompted the error.

janicduplessis commented 4 days ago

I will take this!

Shahidullah-Muffakir commented 4 days ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

We are trying to migrate the MapView component from using the withOnyx HOC to the newer useOnyx hook.

What is the root cause of that problem?

The current implementation uses withOnyx, which is an older pattern for connecting components to Onyx data.

What changes do you think we should make in order to solve the problem?

In this file: https://github.com/Expensify/App/blob/ba9af8f082e76fc7da666e7f128574761fa97012/src/components/MapView/MapView.website.tsx#L35

  1. Remove the withOnyx HOC from the MapView component.

  2. Add the useOnyx hook inside the MapView component to fetch the userLocation data.

    Add this code inside the component to get userLocation:

const [userLocation] = useOnyx({
    key: ONYXKEYS.USER_LOCATION,
});
cretadn22 commented 4 days ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

Migrate MapView to useOnyx

What is the root cause of that problem?

NA

What changes do you think we should make in order to solve the problem?

https://github.com/Expensify/App/blob/d03066fa134eae652137822d264ce6c22beae184/src/components/MapView/MapView.tsx#L301-L304

We need to remove the withOnyx and use const [userLocation] = useOnyx(ONYXKEYS. USER_LOCATION);

What alternative solutions did you explore? (Optional)

janicduplessis commented 3 days ago

@Shahidullah-Muffakir proposal looks good if you want to take this over.

Shahidullah-Muffakir commented 3 days ago

@Shahidullah-Muffakir proposal looks good if you want to take this over.

@janicduplessis ,Sure, I can submit a PR. Thank you.

Shahidullah-Muffakir commented 3 days ago

I removed the userLocation from the props and fetched it from the Onyx store using the useOnyx hook. This change caused a TypeScript error because userLocation was still listed in the MapView props. You can see the relevant code here: MapView types.

To fix the error, I had to remove userLocation from the props types. However, these props types were also used in other files: MapViewImpl. Hence I changed withOnyx to useOnyx in all these files.

Since only MapViewProps was needed in the types file, I realized there was no need for the types.ts file in the MapView folder. Instead, we can directly import MapViewProps in the MapView components.

Here are the changes I made:

Migrated from withOnyx to useOnyx in three MapView components. Removed userLocation from the props types since it’s no longer needed. Deleted the types.ts file and directly imported MapViewProps.

Here is the PR:https://github.com/Expensify/App/pull/50443 Thank you.