[Tracking] Expensify/App cleanup + style guide + README improvements

[marcaaron]

Problem

Expensify/App, Onyx, and our JS styles are inconsistently enforced in the Expensify/App repo. This can happen for many reasons, but one obvious one is that we just haven't written down in our style guide clear examples.

Why is this important?

Incorrect patterns will create more incorrect patterns and general confusion about the "right way" to make a change or implement a feature. Every Expensify contributor should ideally have what they need to be able to champion our code styles and philosophies when reviewing any contribution (internal or external). But this can be difficult without a clear guide to refer to.

Solution

Let's do a quick audit of various things that need clarification and clean them up by creating an issue for each best practice.

The assignee will:

1. Figure out why we have chosen to do things this way
2. Update the style guide with a clear explanation and make sure that it can easily be linked to (if necessary)
3. Update the Expensify/App codebase to eliminate any inconsistencies

Here's a list of things that need to be addressed and can be broken out into other issues:

• Cleanup usages of null coalescence operator #5985
• Cleanup Optional chaining #5986
• Cleanup usages of Onyx directly inside a view #5998
• Fix all inline named exports. Export everything at the bottom of a file. #6011
• Remove usages of compose() with only a single method #6000
• Cleanup component's displayName properties so that functional components all have it and class components do not #6002
• Clean up propTypes imports not usIng camelCase #6004
• Cleanup native array methods prefer _ #5989
• Cleanup any promise chains off of actions inside views #6092
• [HOLD] Cleanup usages where early return is preferred #5995
• No props destructuring for functional components #6250
• Eliminate dependency cycles #6251
• Standardize on import * as Something when modules only have named exports #6072
• Make sure that we are always using Onyx.merge() over Onxy.set() except for when absolutely necessary

Feel free to suggest other things to this list!

[johnmlee101]

Can this be aided with any linting rules?

[tgolen]

We should definitely try to use linting rules whenever necessary, but for a vast majority of these, there are no lint rules for them.

[marcaaron]

I like the idea of looking into some custom lint rules for things like using an array method instead of _. Especially if they direct you to the style guide to also learn why we prefer to do things one way over another. I don't have much experience with writing them so maybe others have ideas on if it's practical.

[kidroca]

There are existing eslint rules for

• prevent null coalescing operator: https://eslint-plugin-es.mysticatea.dev/rules/no-nullish-coalescing-operators.html
• prevent optional chaining: https://eslint-plugin-es.mysticatea.dev/rules/no-optional-chaining.html

There are rules against using native array methods like foreach, filter, map, reduce and so on. Though then won't be able to suggest to use _ instead

Here are some more rules that I think match Expensify current convention

• no get/set https://eslint-plugin-es.mysticatea.dev/rules/no-accessor-properties.html
• no async https://eslint-plugin-es.mysticatea.dev/rules/no-async-functions.html

[kidroca]

Regarding "Make sure that we are always using Onyx.merge() over Onxy.set() except for when absolutely necessary"

This is not just an Expensify/App problem, every future consumer of Onyx can have the same problem as mixing merge and set can lead to race conditions: #5930

In that case IMO it would be best to expose only one method for updating Onyx: #5930 (comment)

[kidroca]

• [HOLD] Cleanup usages where early return is preferred #5995 - Shopify have a custom rule prefer-early-return: https://github.com/Shopify/web-configs/blob/main/packages/eslint-plugin/docs/rules/prefer-early-return.md

[kidroca]

• Cleanup any promise chains off of actions inside views

To prevent this the action should not return a promise
Cleaning up the chain or ignoring the promise of a promise returning method (an async method) is considered an anti pattern as it leads to swallowed errors (Unhandled promise rejection). Many editors would warn you about it, if the promise is handled and the user should not be interacting with it - it should not be exposed to the user.

[roryabraham]

Perhaps out-of-scope, but I've found that there's a lack of consistency in the order in which modules are imported. I would love to impose the sort-imports ESLint rule, and perhaps even absolute import paths.

Also we should nix lodashGet and consistently use _.get instead – one fewer dependency 😄

Edit: eslint-plugin-import seems like maybe a better option than sort-imports

[marcaaron]

@kidroca Thanks for those insights! I think some of those eslint rules definitely make sense for us to add.

mixing merge and set can lead to race conditions

That's a good point. I guess we can keep discussing on that issue, but sounds like the race condition issue might not change whether we should prefer set() or merge(). I think the point we want to make with that one is mostly that we should use whichever is more appropriate.

To prevent this the action should not return a promise

This gets a little tricky sometimes since when actions are referenced by other actions we sometimes need them to return promises, but I like where that idea is going. Probably in those cases we are missing opportunities to move more logic to the API layer.

[marcaaron]

nix lodashGet and consistently use _.get instead – one fewer dependency

@roryabraham that's come up a few times so I think we should just do it!

[marcaaron]

Related to all this I was trying to come up with a graphic to maybe help give a high overview of the app architecture.
Thoughts on adding something like this to the readme? Any changes I should make?

[Luke9389]

I think this infographic is really helpful! Nice job!

Questions:

1. should we have a two-way arrow between expensify.com/api and server data? (to signify read and write's?)
2. Why do we have the actual vs the optimistic response? We may want a little more explanation around that.

[marcaaron]

@Luke9389 Good points thanks!

Why do we have the actual vs the optimistic response?

Good question. I think the guidance could maybe be clearer in the README.
@luacmartins recently asked the same question and this is all I could find...

An optimistic response is a kind of UI enhancement which ensures that views update fast without waiting for a response back from the server. This is useful when we are creating, updating, or deleting something. So, if we fail to create or update data it should also be "rolled back" in some cases. But in others we will keep trying to make the change until it succeeds e.g. report comments with spotty network connection.

[parasharrajat]

I would love to see the sorted imports.
👍 #5984 (comment)

[marcaaron]

If enough people want to sort the imports then we should do it! Personally, I find arbitrarily forcing things to be alphabetized leads to tedious manual style fixes and not sure if it's worth it. Auto-fixing might solve that concern for me, but auto-fixing isn't going to be a part of every contributors workflow.

[marcaaron]

Ah bummer so _.get() actually does not work with paths like this 🙃

_.get(someObject, 'property.nestedProperty', 'default');

It will do arrays, but not single string with . separator. So I think probably we should stick with lodashGet() for now.

[roryabraham]

Another suggestion for something we could standardize + add to the style guide:

Named exports should be grouped together instead of spread throughout a file as individual named exports

// Bad
export const X = 'Hello';
export const Y = 'world!';

// Good
const X = 'Hello';
const Y = 'world!';
export {
    X,
    Y,
};

That way, it's easy to see all the named exports in a module. We are prettymuch standardized on that but might be good to document it anyways because I just saw it come up in a PR review.

[marcaaron]

Unsure how others feel about this, but one thing that could be better enforced is having "actions" always use a specific syntax. Maybe even call these files *Actions?

import * as PolicyActions from './lib/actions/PolicyActions';

PolicyActions.create();

Compare this with a file where we are doing something like this

import {create as createPolicy} from './lib/actions/Policy';

createPolicy();

I generally find it much harder to resolve where these thing happen in the second example. It's much easier to global search for something like PolicyActions.create() vs. create() or createPolicy().

Edit: Also seems like a place where there is a subtle lack of consistency

App/src/pages/workspace/WorkspaceSettingsPage.js

Line 16 in 1870921

import * as Policy from '../../libs/actions/Policy';

App/src/pages/home/sidebar/SidebarScreen.js

Line 30 in 1870921

import {create, isAdminOfFreePolicy} from '../../../libs/actions/Policy';

[roryabraham]

Great suggestion @marcaaron! Let's standardize on this syntax:

import * as PolicyActions from './lib/actions/PolicyActions';

[marcaaron]

Been thinking about this one a bit...

Make sure that we are always using Onyx.merge() over Onxy.set() except for when absolutely necessary

I am proposing something like this for the docs

Should I use merge() or set() or both?

• Use merge() if you want to merge partial data into an existing Array or Object
• Use set() if we are working with simple values (String, Boolean, etc), need to completely overwrite a complex property of an Object, or reset some data entirely.

I might even go so far as to say that Onyx.merge() should throw an error if you try to pass it something that is not one of these:

• Object
• Array
• null
• undefined

If you have something simple I think there is virtually no reason to use merge()? 🤔

[kidroca]

This seems like one more reason to have a single method for updates - it can work with primitive and referential values and handle them appropriately instead of trying to explain that in the documentation where it could be missed or forgotten

Another thing that seems odd is that merge expects null or undefined. How are these values treated? - they can't be merged or at least it wouldn't change the target object. Is Onyx.merge('someKey', null) the same as Onyx.set('someKey', null) ?

[tgolen]

RE: import * as - I agree with all that was said about this @marcaaron but I think what I'm struggling with is there doesn't feel like a compelling argument why this would only be applied to actions. I think the reasons given for wanting to use this pattern will apply to any import and not just actions. Then, the discussion becomes... should we always import * as for EVERYTHING? Or for what cases would we not allow it specifically?

I think one of the biggest reasons we've always opted for importing specific things is to take advantage of "tree-shaking" technology that would keep from importing everything. However, in practice we don't write code that isn't used somewhere, so the tree-shaking isn't something we need. Just some things to consider!

---

For the merge vs. set discussion, I haven't really weighed in because I was interested to see where it would go. I think that when there are two methods, we will always struggle with not only advising which to use, but also enforcing it. I think throwing an error based on value type is a good enforcement method, but I am not totally sold on value type being the reason to choose one over the other. I think that's a hard determination for someone to make because the same piece of data could be stored in multiple ways (ie 0, false, {something: false}, {something: 0}), so now they have to decide what the type of data is that they want to store, and then find the matching method.

I like @kidroca's proposal of a unified method because I think that makes the usages much more explicit (and also a bonus if it solves race conditions, but that's a separate discussion). It is easier for someone to pass ANYTHING as a value, and then they just need to decide the update strategy (which is really the thing that matters).

[marcaaron]

there doesn't feel like a compelling argument why this would only be applied to actions. I think the reasons given for wanting to use this pattern will apply to any import and not just actions. Then, the discussion becomes... should we always import * as for EVERYTHING? Or for what cases would we not allow it specifically?

That's a good point. I would also be fine with adopting the practice where all modules with "named exports only should" have a single import since it makes code more readable and we are not really benefiting from tree shaking when referencing our own code. Tree-shaking benefit seems to imply there is dead code (and we shouldn't have any in theory 🤞).

[roryabraham]

I posted a PR to implement my above suggestion banning props/state destructuring.

[roryabraham]

Another suggestion ... no ternaries in JSX:

// Bad
const MyBadComponent = (props) => (
    <>
	      {props.shouldRenderA
	          ? <A />
	          : null}
	      {props.shouldRenderB
	          ? <B />
	          : null}
    </>
);

// Good
const MyGoodComponent = (props) => (
    <>
        {shouldRenderA && <A />}
        {shouldRenderB && <B />}
    </>
);

[tgolen]

I definitely agree with your bad example, but I think there is still a valid use case for ternaries, such as this:

{thisThing
    ? <A />
    : <B />
}

[roryabraham]

Okay, so maybe "No ternaries in JSX where the second half of the ternary is null"

[tgolen]

Yeah, I like that. I wonder if there is ever a valid case for any ternary to have the second half be null?

[tgolen]

I guess I can find quite a few of them in our code, but maybe we just stick to JSX for now.

[roryabraham]

Looks like there's a pre-existing eslint plugin/rule for this: https://github.com/divyagnan/eslint-plugin-ternaries/blob/master/docs/rules/no-null-ternary.md 🎉

[marcaaron]

I wonder if there is ever a valid case for any ternary to have the second half be null?

I think yes, because React will try to render the left hand side in some cases e.g. if it is a 0. But there are other solutions to that like ensuring you have a boolean for your left hand operator.

{items.length && <List items={items} />} // prints 0

[Luke9389]

Definitely on board for 'no-null-ternaries'. By 'no ternaries in JSX' are we really saying no ternaries in the return method? In general, I think they're a pretty useful shorthand that work fine for other logic happening elsewhere in the component.

[marcaaron]

are we really saying no ternaries in the return method

I think we are saying don't have them in JSX. But if you are returning null instead of JSX I might ask why you are not conditionally rendering it instead...

const MyComponent = (props) => {
    if (!props.someCondition) {
        return null;
    }

    return (
        <View>{...stuff}</View>
    );
}

vs.

{someCondition && <MyComponent />}

[Luke9389]

Ah OK, I think I should have been more clear. What I was wanting to clarify is that we can have ternaries in a .jsx file, but we don't want them returning actual JSX components, correct?

[tgolen]

@marcaaron I'd still say there is a valid case for your example pattern. I like using that pattern when data is being loaded and I don't want to display anything (including a spinner). However, outside of "data being loaded", I would agree that pattern isn't a good one.

@Luke9389 I think what we're saying is to not allow ternary-nulls inside the part that is returned from render():

render() {
    // this part is OK
    return (
        // this part is not OK
    );
}

.jsx files are only used in web-e, but the same rule would apply in that repo.

[kidroca]

IMO early returns in componentDidUpdate or anywhere else we run a side effect due to certain conditions is harmful

Check this thread: https://expensify.slack.com/archives/C01GTK53T8Q/p1636393736077100

Let's say we have a code like this

componentDidUpdate(prevProps) {
        if (!prevProps.visible && this.props.visible) {
            this.input.focus();
        }
    }

we're now forced to change the signature to early return

componentDidUpdate(prevProps) {
    if (prevProps.visible || !this.props.visible) {
        return;
    }
    this.input.focus();
}

But if we need to include another side effect we'll have to get rid of the early return and revert the code

componentDidUpdate(prevProps) {
        if (!prevProps.visible && this.props.visible) {
            this.input.focus();
        }

        if (prevProps.message.length < this.props.messageLength) {
            this.doSomething();
        }
    }

---

Early return can be handy for functions that return something (e.g. other than void/undefined)
Usually a boolean function that can return false at the start
Or a filtering function that return []

But for code that is causing side effects it can be confusing
A condition like !prevProps.visible && this.props.visible) is clear - it's a specific transition from old prop state to new prop state that causes a side effect

Sometimes more side effects would be added and they might need to run concurrently (like in my last example) in that case the early return code will be modified and a bug can be introduced if it's not inverted correctly
The least it's a change that could've been skipped

[marcaaron]

a bug can be introduced if it's not inverted correctly

I can definitely see how someone could make that mistake, but unsure if this is actually a problem we've run into apart from people having opinions about it. A few ideas for solving your concern might be...

• Write a test if you are unsure whether logic has been inverted correctly or are afraid it will be broken in the future
• Use parens and invert an entire statement that way it is easier to invert again later (tbh dislike this practice because I find these kinds of statements hard to understand)
• Refactor by extracting the conditional into a method so that the intention is clearer (my preference)

e.g.

componentDidUpdate(prevProps) {
    if (!this.canFocusInput()) {
        return;
    }

    this.input.focus();
}

canFocusInput() {
    return !prevProps.visible && this.props.visible;
}

Will just need to be changed to...

componentDidUpdate(prevProps) {
    if (this.canFocusInput()) {
        this.input.focus();
    }

    // other side effects...
}

[kidroca]

An early return is suitable for functions that aren't supposed to trigger side effects like shouldComponentUpdate

When we want to trigger a side effect we should strive for clarity for the exact conditions that trigger the side effect

• writing an unit test is great but it won't improve the readability of the statement
• using inverted statements also degrade readability
• extracting a method might work, but it's not as easy to enforce

---

If this code:

componentDidUpdate(prevProps) {
    if (!this.canFocusInput(prevProps)) {
        return;
    }

    this.input.focus();
}

Would have to turn into this code:

componentDidUpdate(prevProps) {
    if (this.canFocusInput(prevProps)) {
        this.input.focus();
    }

    if (this.shouldDoSomething(prevProps)) {
       this.doSomething();
    }
}

Then simply start with

componentDidUpdate(prevProps) {
    if (this.canFocusInput(prevProps)) {
        this.input.focus();
    }
}

[kidroca]

There are cases where we'd like to skip all side effects - an early return comes naturally and conveys that well
It doesn't degrade readability for the rest of the statements

componentDidUpdate(prevProps) {
    if (this.props.disabled) {
        return;
    }

    if (this.canFocusInput(prevProps)) {
        this.input.focus();
    }
}

IMO that's the only case where an early return is preferable in a side effects producing code

[marcaaron]

Let's wait and see if this causes any issues. It should not be hard to modify the custom rule to skip when inside a componentDidUpdate() function body if this creates enough issues for us. I thought early returns were a bit weird at first but trust me it gets easier the more you do it!

[kidroca]

I'm on the latest main and have been spotting this eslint error across the app

This is inside src/libs/actions/ActiveClients.js so why isn't the rule satisfied?

[marcaaron]

so why isn't the rule satisfied

Do you see it when eslint runs in a GH action? Or just locally?

[kidroca]

Do you see it when eslint runs in a GH action? Or just locally?

Locally

[marcaaron]

Ah hmm interesting. Just checked to be sure and doesn't complain for me. Perhaps there is something specific to your environment that causes the rule to show errors in your IDE.

[marcaaron]

Looks like there is only one thing left to cover here which is:

Make sure that we are always using Onyx.merge() over Onxy.set() except for when absolutely necessary

Unsure if we have really landed on the exact guidance for that and the discussion can continue to happen here.

But I think we accomplished a lot here and can close this out. Thanks for everyone's help!

[kidroca]

Ah hmm interesting. Just checked to be sure and doesn't complain for me. Perhaps there is something specific to your environment that causes the rule to show errors in your IDE.

I've traced the problem to file path checks and path separators
Depending on the OS the separator can be a forward a backward slash
For me context.getFilename() resolves to this string D:\Expensify.cash\src\libs\actions\ActiveClients.js
The check below would not match:

/**
 * @param {String} filename
 * @returns {Boolean}
 */
function isInLibs(filename) {
    return filename.includes('/src/libs/');
}

To make it platform agnostic it should be

const path = require('path');

/**
 * @param {String} filename
 * @returns {Boolean}
 */
function isInLibs(filename) {
    const prefix = path.normalize('/src/libs/');
    return filename.includes(prefix);
}

Alternatively all separators can be replaced with / like filename.replaceAll(path.sep, '/').includes('/src/libs/)