Quick Engineering Guide I drafted for one of my teams

20 August 2020

Numbers are not ordered by importance, just for easy reference. As a team-lead I've found myself repeating some tips again and again, so here are some of the most common ones. Some are dead obvious but we've had team members who didn't like rules a lot and they had to explicitly be reminded about a lot of things.

1. Don't create/define a component inside of component

2. when re-setting state based on previous value, use the callback function to take the prev value

   • Incorrect example:

     onClick={e => setShowFilters(!showFilters)}
     

3. keep styles in styled components. Don't use the div's inline styles. It decreases signal-to-noise

4. create an absolute path for modules folder(currently called components), utils, assets

5. start using index files and re-export only what needs to be public

5.1. fix anyS. A lot... 290+ both explicit and and implicit any

6. Don't allow the app to silently work with incorrect state/data! Very important. Thoroughly check for states that should not work and either 1. throw an error(not the best practice) 2. the function that this check is made should return an object with an error field. Then check if after the call we have an error, and if yes, set some error state to display to client if necessary. If not necessary, at least don't execute logic in that incorrect data state(e.g. fields missing, objects undefined). Second choice is better. But throwing an error is still way better than not doing anything at all, allowing the app to function with an incorrect data/state. Remember as "fail early".

7. Don't use ?.someField to execute logic. Following the tip above, better thoroughly check for what you expect to be there, and if not there, either 1 or 2. from prev point. ?. is still fine in some limited cases, such as wanting to check some single deep object if (obj?.obj1?.obj2), then it's fine but I recommend if you're not sure when something is good to be used, just don't use it. It's simpler and safer.

   • Clarification: replacing ?. with field && field.innerField is doing the same thing just longer. It's useless to do that. Re-read the above carefully. Thoroughly check, means to check if !field then do something, set error state, or throw an error. Else do something else, execute logic, render etc. It doesn't mean to explicitly write field && field.innerField.

8. give meaningful names for if statements(or && checks). Not necessary for the execution of the code to work properly but definitely helps comprehension. See variables below. Usually what we do is directly put all the checks in the ifs

   

9. When creating your own types, don't set fields as null or undefined just because you think the external data might come in a certain format. Instead, create the types based on what that given component/funciton needs. You can't predict, and shouldn't, what format the caller of the component/function will have its data. Meaning that if the component/funciton expects to work with some data, and that data to not be null or undefined, then make the type to not allow null or undefined. Then the check for null or undefined automatically, by design, has to happen outside of that function/component where it needs to be. The same goes for the shape of the data. Don't try to predict in what shape the data will come from above. The data above must be properly mapped to fit the lower components. Not the other way around.

10. Read a few articles about atomic design for react. We're not using it, but it explains well a lot of the ways to think properly about structuring your components and how not to.

11. become comfortable with typescript, we've been using it for a very long time. A few examples, would be discriminated union types. Learn and become very comfortable using discriminated union types. Use them often. How, why, when, when not. Generics, become very comfortable with making components with generic data(google typescript react, generic components). Make a few generic components. And of course, become comfortable overall with the concept of generics. Enums.

12. Although we aren't using any testing library, learn about the concept of testing. Learn why and how it helps, see how it will make you think about constructing your components. Why it's really a good idea for the "leaf components" to be "dumb". We have a lot of leaf components that are trying to be too smart, with too much logic, trying to handle too many cases, capable of erroring in many different and unexpected ways and ambiguous behaviour for even the smallest of changes. Please read and think about these things.

13. files in reusable folder must NOT import things from page-specific folders. Also components inside a specific page must not import components from other pages. Still happening in places.

14. comp1.tsx must not import styles from comp2.style.ts. And no, these little things aren't just a whim, when used, want to modify something, and can't because it's used somewhere else unexpectedly and silently breaking stuff

15. Don't modify props... from inside component. I've seen this at least a couple of times.

16. when props start getting more than 6-7, start thinking about grouping them into meaningful objects, or even splitting the component into multiple components

17. Don't modify/mutate function's parameters. Use the parameter values and create a new value, then return that new value, without modifying the previous ones. Use https://doesitmutate.xyz/ if using array functions.

18. Learn about dependency injection. Not just learn what it is, but become comfortable with the idea and its benefits. How and when to apply it. What does DI mean in the context of React apps, vs what does it mean for a normal e..g. backend app with classes. SOLID

19-21. Removed project specific details

22. OrganizationCardProps, Organization | any, is basically any. Don't write SomeType | any.

23. Try not to use names such as name1, name2, name3 when possible.

24. move usePrevious in reusable

25. just like component.style.ts, think about creating component.types.ts files. Also types.ts, without a component name, this means that those types are free to use in all files inside that folder and deeper. Also when in reusable, e.g. /reusable/types.ts/ these types can be used anywhere.

26. although JSX is very powerful and allows us to have all kinds of logic next to the html, try to think about ways to simplify what's in it and not have too many checks, maps or props. Logic can be moved up in many cases. Maybe not always but often

27. Inconsistent naming. getUser in one component gets the logged user id, getAdmin does the same in another. getUser is not descriptive, what user? It can be any user, why have to investigate and remember? Name it getLoggedUser both places. Also getAdmin is not descriptive enough and as of late also incorrect

28. leave comments next to hooks where eslint says there are missing arguments. Is it on purpose? Why is it?

29. const and enums over flying strings

30. specific const a: 'a' | 'A' types over general types in this case string. String only when we don't know exactly the different values that might come. When we know exactly the variations of the values, then give the specific value type.

31. any time you create a function, try to make it do as little as possible. The strong cohesion, loose coupling principle. If you think the function needs to do some more, maybe split it into two or three and chain them together. E.g. (definitely just a small example, I've seen way, way more complex and convoluted ones), the disconnectAccount function. If we think we need to 'delete' fields, we shouldn't do/mutate that hiddenly/silently after the params have been passed to the function. It's an unwritten rule but it's an important one. Even better, create a custom type that will not allow having these fields that we want to delete later on. This way we can be compile-time sure that it's okay.

32. explicitly give names to logic/functions for easier comprehension, also often times this way we catch "code smells" because explicitly naming the functions makes us think about how to combine the code and sometimes we find out that what we do too much at one place. E.g. we name the arrow fn "doSomething" then we see we doSomething, doSomething1, doSomething2 etc. And by naming it we also see how to split it more easily. And then only if we need the inner working details we can go to the definition of the function and see what's it all about. It seems like a miniature thing but when you read code a lot, these little things add up very quickly. E.g. .then(doAction) vs the below code. If you find it hard to think of a proper name, it means it probably should be split

    

33. don't be afraid of TODO/FIXME type comments. Think something needs a little more work? Leave a TODO or FIXME, easier for colleagues to know at what state the code they are working with is at.

34. logically group longer JSXs into smaller and meaningful pieces with good names. E.g. the clientcontent ones can defninitely be made a little more readable

35. read and try to understand apollo's update functions, don't try to replicate its behaviour with set states and useeffects. It only creates more unexpected behaviour and becomes difficult to change. Yes it's not the most intuitive but it's still more intuitive than duplicating, trying to replicate its behaviour with state and effects. Also the new 3.0 version of Apollo is a lot better at that.

36. don't use ! assertion operators. Very bad practice. Try to think why. If TS says it might be null or undefined, and we expect it to not be, then do an error check and follow the tips above.

    An exception that TS cannot track. Still don't use !s

37. avoid casting as much as possible. The oid, uid from router query is fine for example but most other places(probably all) is not. If changing from 1 type to another is unavoidable, or really necessary create a mapping function. Example

    const mapXToY = (x: X): Y ⇒ {... return y}
    

38. for slightly more complex functions, it's better to explicitly define the return type to avoid unexpected mistakes. If it's a dead simple function, it's ok

39. don't pass whole objects and tons of fields that are not used to components or functions. Only pass the fields that the fn/component needs. YAGNI. This also saves up time from future refactorings. When these unnecessarily passed objects get refactored, now all the functions and components need to get refactored as well

40. most if not all articles on Medium Eric Elliot's profile are sensible and explain in good technical detail whys and hows of different technical issues