JavaScript (JS) continues to devour an ever increasing portion of the software universe. It has a forgiving nature and few restrictions - allowing nearly anyone to get started with few requirements to understand complex programming concepts.
However, this trait becomes challenging when the need to manage the complexity of intricate JS applications arises, as those tools and paradigms aren’t baked into the language and standard toolchains. This leaves use leaning on tools and practices instead.
You’ll likely have to create and review JS as part of your day to day. Below is some guidance on what to look for in a PR and keeping master green.
Here are some core concepts to consider creating or reviewing a PR. These can be subjective but always aim to have the code be understandable to the most junior person on the team.
-
Is the intent understandable? Is the role, purpose, and function of each new file or change easy to infer?
-
Is the execution path understandable? Can you easily understand the flow of actions for the components being presented? If it’s difficult to understand what does what, why, and when, consider leaving a comment.
-
Is the data flow understandable? We utilise a number of side effect tools and components. Is it easy to infer where things come from, what happens if something goes wrong, and how data is pushed back up?
Separation of concerns was a popular approach a decade ago, often seen in “MVC/MVVC” structured applications. With React, we try instead to think of things as complete feature components, with everything needed to reuse that feature as close as possible.
Try to create feature folders and store the components, graphql, and test files in those places. Lift them out into the general “components” folder only when they have a re-use purpose.
It can be tempting to create a large number of small components or clever composition to do things like store UI state in a component. Instead, try to simply use a React class component and use its state and lifecycle methods. This colocates all the logic needed to understand the component in one place.
We use static types to catch mistakes and guide our development. It’s important that for all new feature work, // @flow be added to the top.
If reviewing, be patient with the author around this, static types are often a new concept and can seem frustrating at first, and can feel like a barrier to rushing through features. Empathise with them and understand that it’s difficult to internalise that this will likely reduce future bugs and revisiting incomplete features.
Additionally, look and correct for the following behaviour:
- Lack of typing on dynamic/complex areas of code (where it would be most valuable)
- Use of ‘Function’ or ‘Object’ or ‘Any’ as a type.
- Overuse of the existential type (*)
- Exporting types, these should be local-to-file or global
We generally lean on integration tests for all side-effect related outcomes. Often this means working with Apollo’s flow. Check that queries, mutations, and side-effects are tested.
A special mention must be made to everyone’s favourite threateningly named component. This is a legacy component within nest-ui that attempted to recursively generate forms by inferring intent from the shape of data it received and a configuration file.
While the approach was clever, it contained a number of irreconcilable faults. No new work should use this and if your feature is within one of these sections, it’s considered good practice to extract it out into standard JSX components.
Below are a number of examples of “quite good PRs” to get a feel for what’s right. Notes are given with each one to explain their positive attributes.
neste-dot-com: Order fees slider by days rather than fee A small update that adds functionality, while also adding types and uses a class constructor to store logic.
Nest-UI: Add a fallout reason when BPI switches to inactive A larger PR. Good colocation of files and logic. Well integration tested.