Skip to content

Coding Guidelines

Jump to bottom
Rita Sousa edited this page May 19, 2023 · 14 revisions

The following code guidelines should be followed by contributors to this package.

Please do not file issues about these guidelines.

Exporting modules

  • All public types and modules must be exported and accessible through the root index file of the package they are a part of (the index test snapshot file of each package is used to test for that).

    • If a type is being used in the definition of another type that is public and is being exported, ensure it is exported as well.

  • Make sure no privately used modules end up being exported from the package.

  • If a type defined in a @types package is used in a publicly exported type, it is essential to add the @types package as a dependency of the package (not in the monorepo!). Otherwise, the @types package can be installed in the monorepo as a devDependency.

  • Action types, reducers, middlewares and entitiesMappers should be exported under a name that refers to the area. For example:

    • bags action types are exported as bagsActionTypes;

    • bags reducer is exported as bagsReducer;

    • bags entities mappers are exported as bagsEntitiesMapper;

    • bags middlewares are exported as bagsMiddlewares;

Importing modules

  • Use relative imports to import modules inside the same package instead of self-referencing the package.

  • When using relative imports, prefer imports directly to the file instead of importing from the index when possible.

    • This is to avoid having files that import other files unnecessarily and might lead to circular dependencies. Importing from indexes is most problematic when they export a lot of different modules (i.e. the root index files). They are fine if used sensibly.

Types

  • All code files (i.e. files inside the src folder) must be in typescript.

  • Prefer using type to define new types instead of using interface as they are more flexible.

    • One drawback of type is that when using type intersections to extend a type, conflicts might not output errors. For example, if a type defines a property x as a number and another type that extends it defines a property x as a string, the intersected type will be calculated as type never while if an interface was used, an error would be raised. To deal with this, we will need to use the Omit type when defining the intersection to remove the property and then redefine it with the new type.

    • One drawback of using interface is that it allows declaration merging, i.e., the interface can be extended by adding other declarations with the same name. This can lead to some difficult issues so we should avoid it most of the time. So our recommendation is that when in doubt and whenever it is possible use type.

  • Ensure that the types are defined and exported in the correct package. For example, a type that is used solely in the @farfetch/blackout-redux package should not be defined and exported from the @farfetch/blackout-client package.

  • Avoid abusing any. There are times when this type is necessary but most of the time we can use a more specific type. If there is not a more specific type we recommend the usage of unknown instead of any.

  • Make sure the types of selectors created by createSelector are inferred correctly as most of the time they are not. If not, then declare the correct type of the selector including the return type of the function.

  • Most of the time there is no need to define return types, as we recommend having typescript infer the type. Exceptions should happen to ensure that no value of an incorrect type is being returned when the function is not trivial to implement.

  • If there is a need to create a type for a value that is supposed to conform to a public standard (example: JSON Schema) make sure to check if there is a @types package available that can be used instead of defining new types.

  • When possible refer to other types instead of hardcoding a type. For example, when typing a function that receives the user id as the first parameter which is a number, do not type the function like this:

    function doSomethingWithUser(userId: number);

    but define it like this, instead:

    function doSomethingWithUser(userId: User['id']);

  • Be mindful not to redefine a type multiple times, if the types are the same and are being used by different modules. Follow these guidelines:

    • Find if the necessary type already exists in the package. For example, by searching for types that contain some of the properties needed we can find that those types (similar) already exist.

    • If there is another type that matches exactly, check if it makes sense to have a more generic name in order to be shared by the modules. For example, if there is an Environment type that can be reused by many modules, use it instead of defining a specific type or a different alias for that type like <Area>Environment (CheckoutEnvironment for example).

    • If there is another type that matches exactly but it makes sense to be treated as different types, then create an alias for that type instead of redefining it. For example: BagOperationChange and CheckoutOrderOperationChange are aliases to the same base private type OperationChange.

  • Use @ts-expect-error, @ts-nocheck and @ts-ignore directives sparingly.

    • There are some valid use cases (e.g. having an invalid value to test the errors) for them but most of the time they should not be used. When they are needed, guarantee that a clear description is provided.

  • The action parameter in redux reducers should be typed as AnyAction and not as the expected action type since reducers can be called with all actions.

Naming

  • Keep consistency with the names used throughout the code.

    • Especially for names that are part of the public API like in clients, actions, selectors, types...

  • Do not use generic names for public modules/types unless they represent something that is not specific to a feature. For example, use names like BagOperation and CheckoutOrderOperation instead of just Operation as those names are more meaningful and only used by those areas.

  • Enumerations should use PascalCase naming convention for both the name of the enum and the enum members. For example avoid this definition:

 // Avoid this
 enum MY_ENUM {
   VALUE_1,
   VALUE_2,
   VALUE_3,
 }

but use this definition, instead:

 // Use this
 enum MyEnum {
   Value1,
   Value2,
   Value3,
 }
  • Use plural names for enums that are used as bit fields or flags and singular names otherwise:
 // This is treated as a bit field or flags enum (You can have more than one interest at the same time), so it should be plural
 enum Interests {
   PlayFootball = 1,
   GoShopping = 2,
   RacingCars = 4
 }
 // This is not a bit field enum (An order cannot have 2 statuses at the same time), so it should be singular
 enum OrderStatus {
   Pending,
   Accepted,
   Canceled,
 }

  • selectors that return the loading state related to a request that will load a group of entities (for example, from a request to fetchUserOrders, fetchUserAddresses, etc...) should be named as are<Entity>Loading (for the UserOrders example, its loading selector should be named areUserOrdersLoading and not isUserOrdersLoading).

  • To name clients, we use a name that contains the HTTP method of the client as the prefix:

    HTTP Method Name prefix Example
    GET get getOrder
    POST post postBagItem
    PUT put putUser
    PATCH patch patchReturn
    DELETE delete deleteToken

  • To name redux actions, we try to use the following mappings from the HTTP method of the client to get the name prefix:

    HTTP Method Name prefixes Examples
    GET fetch fetchOrder
    POST create, add addBagItem, createReturn
    PUT set, update setBagPromocodes, updateUserSubscriptions
    PATCH update updateBagItem, updateReturn
    DELETE remove removeBagItem, removeCheckoutOrderItem

    There might be exceptions to these rules depending on the semantics of the action. When the name is not a perfect match please highlight the reasons not to follow these mapping rules on your PR.

File names

  • Make sure file names are correct (without any lingering spaces or misspelled names).

  • Ensure the modules are defined in the correct files:

    • types should be defined in a .types.ts file

      • Exceptions could be made for types that are used in test files or that are very simple and private which can be defined directly in the file it is being used in.

        • Mostly for temporary types used internally.

    • Fixtures should be defined in files that end in .fixtures.ts inside folders named __fixtures__.

    • Test files should be defined in files that end in .test.ts inside folders named __tests__.

    • When in doubt, look for similar implementations in other areas for guidance.

Redux

  • Selectors that return values that are normalized in the store, should return them denormalized when possible.

    • Take special consideration to use createSelector to return a new object only when the entity or its sub-entities have indeed changed.

      • Red flags for createSelector:

        • Do not use an input selector that returns the whole state of the app, as the selector will not memoize correctly. The state reference can change when any value in the state has changed, discarding the memoization unnecessarily.

        • Avoid calling a selector that is defined with createSelector inside another selector with different arguments as it will prevent the correct memoization from taking place. For example, the selector to return a wishlist should not call the selector to get a wishlist item for each item of the wishlist if the get wishlist item selector is defined with createSelector.

  • Not all requests might need to be stored in the redux store. If a request is likely to not be performed by different components that are shown at the same time, we can avoid saving the state in redux but a hook must be provided that manages the state of the request internally so that consumers can obtain that state.

    • Examples of requests that do not need to be in the store: Requests for data that is ephemeral like "return pickup requests" or "order available activities"

    • Examples of requests that can be in the store: These are requests for data that might persist longer in a user session, like "bag", "wishlist", "orders" or "user".

  • State for requests that accept a query parameter:

    • For requests that might accept a query parameter, consider if it might be necessary for the consumer to have the state segregated by query instead of having only a single value for the last request performed. If the state needs to be segregated by query, the selectors will need to receive the query as a parameter. Besides this, a hashing function should be used internally by these selectors and by the action to get the hash that will be used to find the correct state in the reducer. We do not want to force the consumers to generate the hash themselves unless there is a strong reason to do so.

  • Consider if reset state functions should be provided for the consumer to reset the state of some requests.

    • If provided, they should reset both the state and if applicable, the entities.

  • Selectors that do not return a computed value, i.e., return the response from the endpoint should return undefined when there is no data fetched from the server. This is to help consumers distinguish when there is a need to request data from the server or not.

  • Reducers that deal with state that is related to the current user (orders, addresses, bag, wishlist, etc...) should be reset by listening to the LOGOUT_SUCCESS action.

    • For entities an entitiesMapper for the LOGOUT_SUCCESS action should be provided as well.

  • Action objects:

    • Meta parameters of actions should be objects containing the meta parameters passed on the meta property of the action.

    • Payload of actions should be passed on the payload property of the action and can be other things than an object if it makes sense for the action (For example, the action can contain an array on the payload property).

  • Entities

    • To decide if an entity should be created or not, evaluate if it can be shared (now or in the future) with other entities. If not, do not create an entity and place the result inside its state slice directly.

    • Always use the normalizr package for entities generation with a dedicated schema.

Dependencies

  • Make sure that the runtime dependencies of a package are defined inside the dependencies section of the respective package.json of the package and not in the package.json of the monorepo.

    • An exception must be made for packages that might be used by the applications themselves (like react) which should be specified as peerDependencies of the package instead of a dependency to avoid having duplicated packages installed in the node_modules of the consuming application.

  • The monorepo should only contain devDependencies which are used for management tasks of the repo (jest for testing, lerna for publishing, etc...).

    • Be mindful of packages that are installed as devDependencies of the monorepo but are also necessary at runtime for a specific package. They might not fail when running tests since they will use the hoisted package of the monorepo but will fail at runtime when installed in another app that does not contain the required package.

  • Do not add a dependency if the desired functionality is easily implementable.

  • If it is necessary to add a new dependency, it should be well-maintained and trustworthy.

Tsdoc

  • Ensure that module functionality is concisely described and any problems highlighted. For example, if there is a client to retrieve orders for a registered user and another client to retrieve orders for a guest user, highlight that fact in the description and point the user to the other client if that was his intention. Something like this (definition of the client that fetches guest orders only): "Fetches orders for guest users. If you want to fetch the orders for registered users use the 'XXX' client, instead.".

  • When documenting functions, make sure the global description and the descriptions of its @param and @returns directives match the definition of the function.

    • @returns do not need a - before the description but @param does.

  • Make sure the format is aligned with the tsdoc format and no jsdoc tags which should not be used are inserted (like definition of the type in a @param directive as tsdoc will infer the type from typescript).

Errors

  • Make sure that the errors thrown by clients and action factories are of the type BlackoutError. There is a toBlackoutError function in the client package that takes care of converting an Error instance to a BlackoutError if necessary.

  • Do not throw values that are not errors like throw 'error message' instead of throw new Error('error message')

Hooks

  • Do not ignore or disable the warnings generated by the exhaustive-deps eslint rule. Analyze if the useEffect is really necessary or could be refactored in a way that all dependencies are used inside the effect.

  • If you need to create a custom hook, follow the conventions used in other custom hooks provided by the react package:

    • If the hook can automatically fetch data, accept an options parameter which contains an option called enableAutoFetch to control this auto fetching behaviour.

    • Data should be returned in the data property unless the hook can provide multiple data from different actions. Returned data should have the same reference unless it has changed (use useMemo for that). If there is no data to return, return undefined. Avoid returning objects that are partially filled with properties from different endpoint calls when possible.

    • Actions should be returned under the actions property.

    • Data and actions returned should be related to the context of the hook call. For example, if you are creating a hook called useOrder which accepts an orderId parameter, the data returned should be from the order whose id is equal to the orderId parameter received. Actions that use the orderId parameter, should omit it from the consumer and use the orderId inside.

      • There are some cases where you might want to allow the user to override the implicit parameter (in this example, the orderId) of the action. If you need to support this case, declare the parameter as optional and it should be the last parameter of the function. This is intentional to make the most common usage of not overriding this implicit parameter less cumbersome. But remember that the returned output of the hook should continue to respect the parameter passed on the hook and not the override.

    • If you can automatically reset data when the parameters change on a subsequent hook call, do it. However, do not do it in a useEffect but instead do it directly in the hook's body. Check useCheckout hook for an example when the passed checkoutOrderId is different than the one that is stored in redux data.

Unit Tests

  • Prefer it instead of test to write your test cases.

  • If there are multiple tests for the same functionality, wrap them in a descriptive describe block.

  • Prefer using a beforeEach that calls jest.clearAllMocks() instead of calling that function manually before each test.

  • Use names for tests that are descriptive. For example avoid naming tests like this:

it('should give an error', () => {
 ...
});

And name them like this:

it('should give an error when then endpoint returns an invalid response', () => {
 ...
});
  • When testing redux selectors and reducers, test with a mocked state that is possible to be reached. For example avoid tests like these:
it('should return the loading status of the request', () => {

  // The state cannot have an error and a result at the same time
  const mockState = {
    isLoading: true,
    error: new Error('dummy error'),
    result: [{ x: 1, y: 2, z: 3 }],
  };

  ...

});

And do this instead:

it('should return the loading status of the request', () => {

  // Error is null (or result could be null as well)
  const mockState = {
    isLoading: true,
    error: null,
    result: [{ x: 1, y: 2, z: 3 }],
  };

  ...

});

  • Avoid expect.assertions(N) when:

    • Your tests are synchronous.

    • You are using promises (in which case, just return the expectation).

    • You are using async/await with try/catch (again, just await the expectation. Check existing tests for examples when in doubt).

  • If the code you are testing uses timer functions (setTimeout/setInterval), use fake timers in your test to keep it deterministic.

    • For code that uses requestAnimationFrame use a custom mock instead.

  • If you need to mock global properties (for example, you need to mock window.alert or console.log) that applies to all tests in your file, do not clean it up in an afterAll block as there is no need to do so (jest will execute each test file independently)

    • This does not apply to tests that are in the same file and you do not want to use the global mocks. In that case, make sure you clean the mocks before the test begins.
Clone this wiki locally