[$500] Create an ESLint rule to detect multiple uses of withOnyx

[tgolen]

Problem

When withOnyx is used multiple times while composing components, it adds overhead to what Onyx has to keep track of and also prevents optimizations like batched render updates from working to their full potential.

Solution

Create a custom ESLint in https://github.com/Expensify/eslint-config-expensify/tree/main/eslint-plugin-expensify which detects when withOnyx() is used multiple times in the same file, and throws an error. The error should give instructions on the best practices which is something like:

Only use withOnyx() once. If there are dependent Onyx keys, they can all be handled in a single instance of withOnyx through the use of selectors.

That should also link to the Onyx readme, and I'm going to write a specific section in there about the best practice that we can link to.

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01f185d4f92edd4f6c
• Upwork Job ID: 1702383017796775936
• Last Price Increase: 2023-09-14
• Automatic offers:
• mollfpr | Reviewer | 26743007
• studentofcoding | Contributor | 26743012

[melvin-bot]

Triggered auto assignment to @mallenexpensify (NewFeature), see https://stackoverflowteams.com/c/expensify/questions/14418#:~:text=BugZero%20process%20steps%20for%20feature%20requests for more details.

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01f185d4f92edd4f6c

[melvin-bot]

Triggered auto assignment to @puneetlath (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

[melvin-bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @mollfpr (External)

[needpower]

Proposal

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

withOnyx() can be called multiple times while composing components.

What is the root cause of that problem?

Sometimes developers do not notice that withOnyx() is already called, and create a new call instead of extending the existing one.

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

Create a new eslint rule in https://github.com/Expensify/eslint-config-expensify/blob/main/eslint-plugin-expensify/ called no-multiple-onyx-calls which will display an error message in case of multiple calls, e.g.

export default compose(
    withOnyx({
        isCheckingPublicRoom: {
            key: ONYXKEYS.IS_CHECKING_PUBLIC_ROOM,
            initWithStoredValues: false,
        }
    }),
    withOnyx({
        isSidebarLoaded: {
            key: ONYXKEYS.IS_SIDEBAR_LOADED,
        },
    })
)(Component)

What alternative solutions did you explore? (Optional)

N/A

[alitoshmatov]

Proposal

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

Create an ESLint rule to detect multiple uses of withOnyx

What is the root cause of that problem?

Feature request

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

We should add a new rule to https://github.com/Expensify/eslint-config-expensify/tree/main/eslint-plugin-expensify to check if withOnyx is used more than once

We can achieve it with something like this:

module.exports = {
    create(context) {
        const callStack = [];

        return {
            CallExpression: (node)=> {
                if (node.callee.name === 'withOnyx') {
                    callStack.push(node);
                }
                if (callStack.length > 1) {
                    context.report({
                        node: callStack[callStack.length - 1],
                        message: "Multiple calls to 'withOnyx' function are not allowed in a file.",
                    });
                }
            },
        };
    },
};

Where we count how many times withOnyx function is being called and error when it is more than once. The message is just for placeholder and we can discuss more elaborate message

What alternative solutions did you explore? (Optional)

[ZhenjaHorbach]

Proposal

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

In some files, withOnyx may appear several times

What is the root cause of that problem?

Feature request

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

I think that we should not stop only at withOnyx
Because in the future there may be other cases when it will be important for us that function is called only once

Therefore, I would like to suggest passing the functions we need to the eslint rule

And the rule might look something like this

Result:

What alternative solutions did you explore? (Optional)

NA

[studentofcoding]

Proposal

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

Create an ESLint rule to detect multiple uses of withOnyx

What is the root cause of that problem?

Feature request for Create an ESLint rule to detect multiple uses of withOnyx

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

We should add a new rule to https://github.com/Expensify/eslint-config-expensify/tree/main/eslint-plugin-expensify to check if withOnyx is used more than once, and use the same pattern and modules like others ESLint files (lodashGet, _ (underscore), and message) with the details below:

1. On CONST.js we add NO_MULTIPLE_ONYX_IN_FILE with our desired details so we can use later on no-multiple-onyx-in-file.js

module.exports = {
    MESSAGE: {
        ...
        NO_MULTIPLE_ONYX_IN_FILE: 'Example: Only use withOnyx() once. If there are dependent Onyx keys, they can all be handled in a single instance of withOnyx through the use of selectors.',
        ...
    },
};

2. Create no-multiple-onyx-in-file.js on eslint-plugin-expensify and use the same pattern modules

const _ = require('underscore');
const lodashGet = require('lodash/get');
const message = require('./CONST').MESSAGE.NO_MULTIPLE_ONYX_IN_FILE;

module.exports = {
    create: context => {
        let withOnyxCount = 0;

        return {
            CallExpression(node) {
                const calleeName = lodashGet(node, 'callee.name');

                if (calleeName === 'withOnyx') {
                    withOnyxCount += 1;

                    if (withOnyxCount > 1) {
                        context.report({
                            node,
                            message,
                        });
                    }
                }
            },
            'Program:exit': () => {
                withOnyxCount = 0;
            },
        };
    },
};

In the above example, the error is reported if there are multiple withOnyx calls in the same file, and reset to 0 when ESLint finishes traversing the file.

What alternative solutions did you explore? (Optional)

If we want to show the error directly on any multiple withOnyx we can use the below approach

const _ = require('underscore');
const lodashGet = require('lodash/get');
const message = require('./CONST').MESSAGE.NO_MULTIPLE_ONYX_IN_FILE;

module.exports = {
    create: context => {
        let withOnyxCount = 0;

        return {
            CallExpression(node) {
                const calleeName = lodashGet(node, 'callee.name');

                if (calleeName === 'withOnyx') {
                    withOnyxCount += 1;

                    if (withOnyxCount > 1) {
                        context.report({
                            node,
                            message,
                        });
                    }
                }
            },
        };
    },
};

cc: @tgolen @mallenexpensify @mollfpr

[mollfpr]

When withOnyx is used multiple times while composing components, it adds overhead to what Onyx has to keep track of and also prevents optimizations like batched render updates from working to their full potential.

@tgolen To clarify the lint only detect multiple withOnyx in the compose function?
E.g.

compose(
    withOnyx({
        A,
    }),
    withOnyx({
        B
    })
)

Or also detect withOnyx inside the other HOC e.g. withPolicy.

compose(
    withPolicy,
    withOnyx({
        A,
    })
)

[tgolen]

I was really only thinking of targeting multiple withOnyx in the compose function (the first example).

I didn't consider about the second example, but I am guessing we don't have very many HOCs that use withOnyx(). It's probably fine for now. I think of withPolicy more like it's own component, just like any other component.

[mollfpr]

@needpower Could you elaborate on your solution to show the error on multiple Onyx calls?

@alitoshmatov Seems nice, but it will be great if we show the error on all other Onyx calls, not just on the first duplicate.

I was really only thinking of targeting multiple withOnyx in the compose function (the first example).

@ZhenjaHorbach Great idea, but I prefer to stick to the expectation above. Also, please don't leave the root cause section empty.

@studentofcoding Could you explain why we need to add Program:exit? Is there any significant different without it?

[studentofcoding]

Hey @mollfpr, we can benefit from adding Program:exit only if we want to include this ESLint to run into our automatic test, so:

• It'll give error for each withOnyx found on all files (as it'll reset to 0, when finishing checking the file)

But, if we want to only run the checking on specific files that opened, then the alternative approach will be sufficient (with also using and extending the pattern that we already have for other ESLint rule)

const _ = require('underscore');
const lodashGet = require('lodash/get');
const message = require('./CONST').MESSAGE.NO_MULTIPLE_ONYX_IN_FILE;

module.exports = {
    create: context => {
        let withOnyxCount = 0;

        return {
            CallExpression(node) {
                const calleeName = lodashGet(node, 'callee.name');

                if (calleeName === 'withOnyx') {
                    withOnyxCount += 1;

                    if (withOnyxCount > 1) {
                        context.report({
                            node,
                            message,
                        });
                    }
                }
            },
        };
    },
};

[alitoshmatov]

@mollfpr You are right we can achieve that by following code

module.exports = {
    create(context) {
        const callStack = [];

        return {
            CallExpression: (node)=> {
                if (node.callee.name === 'withOnyx') {
                    callStack.push(node);
                }
                if (callStack.length > 1) {
                    context.report({
                        node: callStack[callStack.length - 1],
                        message: "Multiple calls to 'withOnyx' function are not allowed in a file.",
                    });
                }
            },
        };
    },
};

Where we report an error on every last instance of withOnyx

[mollfpr]

Thanks @studentofcoding, for the explanation. I think our automatic action is to run npm run lint and I believe it will work without the hook.

@alitoshmatov Could you update that to your proposal? Thanks!

[studentofcoding]

Thanks @studentofcoding, for the explanation. I think our automatic action is to run npm run lint and I believe it will work without the hook.

@alitoshmatov Could you update that to your proposal? Thanks!

Just for another context, the fix on @alitoshmatov above are already apart on my initial Alternative solution here 4 days before @mollfpr

[alitoshmatov]

Updated my proposal - #27463 (comment)

[mollfpr]

Just for another context, the fix on @alitoshmatov above are already apart on my initial Alternative solution #27463 (comment) 4 days before @mollfpr

I believe @alitoshmatov is posting it first.

A couple of proposals seem similar, just different in the logic of showing the error. I think there's no difference on which it's better, so I'll choose the first posted proposal that works.

The proposal from @alitoshmatov looks good to me!

🎀 👀 🎀 C+ reviewed!

[melvin-bot]

Triggered auto assignment to @neil-marcellini, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[studentofcoding]

Just for another context, the fix on @alitoshmatov above are already apart on my initial Alternative solution #27463 (comment) 4 days before @mollfpr

I believe @alitoshmatov is posting it first.

A couple of proposals seem similar, just different in the logic of showing the error. I think there's no difference on which it's better, so I'll choose the first posted proposal that works.

The proposal from @alitoshmatov looks good to me!

🎀 👀 🎀 C+ reviewed!

Although I understand that the order matters, I believe the correctness and thoroughness of details (in this case, using all modules we already used on another ESLint) will also add the point for choosing a proposal.

But, if it's not, then I just want to speak my mind about this, cheers!

[neil-marcellini]

@mollfpr I actually like @studentofcoding's proposal better because the program exit thing seems like a good idea, and more importantly he used the correct error message mentioned in the issue.

In the interest of speed I'm going to hire him now, hopefully everyone feels good about this.

[melvin-bot]

📣 @mollfpr 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

[melvin-bot]

📣 @studentofcoding 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

[studentofcoding]

Thanks, @neil-marcellini

Upon Reading the Readme on https://github.com/Expensify/eslint-config-expensify it's said that the PR should be there, therefore it's ready to review on Expensify/eslint-config-expensify#73 @mollfpr @tgolen

[studentofcoding]

The follow up PR is ready for review @tgolen @mollfpr #27840, thanks!

[studentofcoding]

Hey @MelvinBot the PR is merged yesterday (takes 2 days)

[studentofcoding]

Hey @tgolen, as the PR is merged 6 days ago, and @MelvinBot didn't automatically report it. I believe the waiting time for payment is only 1 day left and tomorrow it should be released right?

[tgolen]

I believe so, but I am not involved in that part of the process too much. @mallenexpensify might know better since he is handling the payment side for this issue.

[studentofcoding]

Thanks for it @tgolen, This is also happening on another PR, in which we have to change the Title of the Issue. If I need to do anything, do let me know @mallenexpensify, thanks!

[studentofcoding]

Hey @tgolen, as the PR is merged 6 days ago, and @MelvinBot didn't automatically report it. I believe the waiting time for payment is only 1 day left and tomorrow it should be released right?

Can we have a guide on this condition @mallenexpensify ? Thanks!

[mallenexpensify]

Contributor: @studentofcoding paid $750 via Upwork, includes 50% urgency bonus
Contributor+: @mollfpr is due $750. Unsure if paying via Upwork or NewDot yet, will know next week.

[mallenexpensify]

confirming payment for @mollfpr internally
https://expensify.slack.com/archives/C01SKUP7QR0/p1696254687480909

[mollfpr]

@mallenexpensify I should be paid via Upwork since I already have the contract for this issue.

[mallenexpensify]

Thanks @mollfpr , I agree, updated the above to reflect you've been paid via Upwork!