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

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

Closed
13 of 14 tasks
marcaaron opened this issue Oct 21, 2021 · 58 comments
Closed
13 of 14 tasks

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

marcaaron opened this issue Oct 21, 2021 · 58 comments
Assignees
Labels
Engineering Internal Requires API changes or must be handled by Expensify staff Monthly KSv2

Comments

@marcaaron
Copy link
Contributor

marcaaron commented Oct 21, 2021

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:

Feel free to suggest other things to this list!

@marcaaron marcaaron added Engineering Internal Requires API changes or must be handled by Expensify staff labels Oct 21, 2021
@marcaaron marcaaron self-assigned this Oct 21, 2021
@johnmlee101
Copy link
Contributor

Can this be aided with any linting rules?

@tgolen
Copy link
Contributor

tgolen commented Oct 21, 2021

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
Copy link
Contributor Author

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.

@marcaaron marcaaron added the Weekly KSv2 label Oct 21, 2021
@kidroca
Copy link
Contributor

kidroca commented Oct 22, 2021

There are existing eslint rules for

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

@kidroca
Copy link
Contributor

kidroca commented Oct 22, 2021

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
Copy link
Contributor

kidroca commented Oct 22, 2021

@kidroca
Copy link
Contributor

kidroca commented Oct 22, 2021

  • 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
Copy link
Contributor

roryabraham commented Oct 22, 2021

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
Copy link
Contributor Author

@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
Copy link
Contributor Author

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
Copy link
Contributor Author

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?

Expensify_App Architecture@2x (1)

@Luke9389
Copy link
Contributor

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
Copy link
Contributor Author

@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...

2021-10-22_10-19-34

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
Copy link
Member

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

@marcaaron
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor

roryabraham commented Oct 22, 2021

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
Copy link
Contributor Author

marcaaron commented Oct 26, 2021

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

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

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

@roryabraham
Copy link
Contributor

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

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

@marcaaron
Copy link
Contributor Author

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
Copy link
Contributor

kidroca commented Oct 27, 2021

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
Copy link
Contributor

tgolen commented Oct 27, 2021

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
Copy link
Contributor Author

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
Copy link
Contributor

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

@roryabraham
Copy link
Contributor

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
Copy link
Contributor

tgolen commented Nov 5, 2021

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
Copy link
Contributor

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

@tgolen
Copy link
Contributor

tgolen commented Nov 5, 2021

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

@tgolen
Copy link
Contributor

tgolen commented Nov 5, 2021

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

@roryabraham
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

Luke9389 commented Nov 5, 2021

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
Copy link
Contributor Author

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
Copy link
Contributor

Luke9389 commented Nov 5, 2021

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
Copy link
Contributor

tgolen commented Nov 5, 2021

@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.

@botify botify closed this as completed Nov 8, 2021
@marcaaron marcaaron reopened this Nov 8, 2021
@kidroca
Copy link
Contributor

kidroca commented Nov 8, 2021

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
Copy link
Contributor Author

marcaaron commented Nov 9, 2021

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
Copy link
Contributor

kidroca commented Nov 9, 2021

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
Copy link
Contributor

kidroca commented Nov 9, 2021

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
Copy link
Contributor Author

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
Copy link
Contributor

kidroca commented Nov 24, 2021

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

image

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

@marcaaron
Copy link
Contributor Author

so why isn't the rule satisfied

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

@kidroca
Copy link
Contributor

kidroca commented Nov 24, 2021

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

Locally

@marcaaron
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor

kidroca commented Nov 25, 2021

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/)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Engineering Internal Requires API changes or must be handled by Expensify staff Monthly KSv2
Projects
None yet
Development

No branches or pull requests

8 participants