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.36k stars 2.78k forks source link

Duplicated state in components due to useOnyx selector #49644

Open kacper-mikolajczak opened 6 days ago

kacper-mikolajczak commented 6 days ago

Problem

Components which maps Onyx collections end up creating duplicated state instances instead of modifying original state.

It bloats the component implementation (DX regression) and can lead to unnecessary increase in re-renders and memory usage (UX regression).

Example As an example, let's check what needs to be done in MoneyReportHeader component to retrieve transactions related to given report:

before

It results in unnecessary unnecessary subscription to entire transaction collection:

Here is a difference between needed state (allTransactions) and subscribed state (transactions):

before-results

Selector improvement

useOnyx has a built-in way to manipulate original state (collection) and subscribe to only a portion of it. Let's add a selector:

const [transactions] = useOnyx(ONYXKEYS.COLLECTION.TRANSACTION, {
    selector: (transaction) => (!!transaction && transaction.reportID === moneyRequestReport?.reportID ? transaction : undefined),
});

Here are results of adding a selector:

selector-results

It does not solve the original issues, because derived state still needs to be present and collection is bigger than expected.

Selector is not able to modify collection as a whole (only iterate over items) which introduces a limitation when dev wants to:

Solution

As an improvement we could extend useOnyx with a functionality to modify entire state at once - it is how most of the libraries work (e.g. jotai, redux, react-query).

For the purpose of the proposal, it's called collectionSelector to distinguish between implementations (Onyx impl). The actual implementation will refactor selector itself, in order not to introduce another method.

Here's how it would look like in our example:

const [transactions] = useOnyx(ONYXKEYS.COLLECTION.TRANSACTION, {
    collectionSelector: (collection) => TransactionUtils.getAllReportTransactions(moneyRequestReport?.reportID, collection),
});

Such implementation removes the need to have a derived state at all. This way component only cares about the state it is interested in:

after-results

Results

By using new selector we've managed to remove derived state from MoneyRequestHeader (diff), thus cleaning up a component a bit and also removed potential re-renders due to other transactions changing.

With other components in mind the collective gain should be higher.

The rendering time in this case won't be different because we are doing the mapping as usual but now in the state management layer instead of component layer (which comes as a plus when it comes to re-rendering as mentioned above).

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

Triggered auto assignment to @muttmuure (NewFeature), see https://stackoverflowteams.com/c/expensify/questions/14418#:~:text=BugZero%20process%20steps%20for%20feature%20requests for more details. Please add this Feature request to a GH project, as outlined in the SO.