docs: reducing complexity

[jesperhodge]

• docs: add adr
• docs: update adrs

[codecov]

Codecov Report

Base: 89.75% // Head: 89.75% // No change to project coverage 👍

Coverage data is based on head (a972ebd) compared to base (4a1bac3).
Patch has no changes to coverable lines.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #220   +/-   ##
=======================================
  Coverage   89.75%   89.75%           
=======================================
  Files         176      176           
  Lines        2761     2761           
  Branches      479      479           
=======================================
  Hits         2478     2478           
  Misses        265      265           
  Partials       18       18           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[bszabo]

Yes and no. Using dependency injection to make it easy to mock out dependencies is essential for testability. One might argue that the inversion of control that comes with that is non-intuitive and makes the code harder to understand. But it's a really sound design and testability principle. We're in agreement if dependency injection is not viewed as increasing code complexity.

[bszabo]

You mentioned the KISS design principle above, but we should equally be adhering to other design principles such as the single-responsibility principle. For example, mixing UI and business logic may be intuitive when first implementing a feature, but may well make the testing harder and may actually be harder to use in the future, as requirements change. This is probably addressed in the open source community practices that you've referenced, but I worry about a blanket statement that we shouldn't structure our code for testability.

Put another way, following the best design principles is good engineering and is good for testability, but it may not be ideal for clarity to someone with three months' experience. I believe a more appropriate metric is that the code is easily accessible to someone with limited domain knowledge, but who has experience following good design principles.

[jesperhodge]

I fully agree with these points. It would be great to find a better wording for this. My motivation for the above statement V. is that I see some unusual patterns in our codebase that I haven't seen used much in React codebases, which I consider bad / unnecessarily complex patterns, and they have been explicitly justified in an earlier ADR with saying that improved testability should justify higher complexity. I am attempting to understand the underlying problem, and it seems to me to have something to do with believing there is a tradeoff - that for higher testability you need to increase code complexity. My experience is usually that higher testability best goes hand in hand with a really good code design. I would also add to your first point that dependency injection as you say is "a really sound design" principle as well, which justifies it. We don't really have the option to do this easily in React, but I want to make the point that we should improve testability along with good design and not instead of good design.
Do you have any ideas how to frame this point in a way that is better and less of a blanket statement?

[bszabo]

Is this a design decision or a coding convention? Sounds more like the latter.

[jesperhodge]

You could call it a coding convention, but it is more of a rule. We use React and React is very good in not being opiniated. However, React docs state custom hooks MUST start with "use" and this is not optional but mandatory. They give very good reasons for it (the link contains it all) and if we don't follow that we don't just create bad code design, but confuse other React developers who look at the code base.

[jesperhodge]

After some time to think: It doesn't matter much how you call it. What's relevant here is that you must be able to distinguish hooks from functions, and this is the formally correct way to do it. I can recommend reading more about it in the beta docs, it is enlightening.

[bszabo]

Ditto on design vs. implementation. You're bringing up good points that will improve the quality of our code, and they belong somewhere. I'm just not sure that this is the place. Perhaps the team needs a coding style document, and these go there. That document might have a section dedicated to React, or perhaps we want a distinct React coding conventions document.

[jesperhodge]

Yeah, I don't have a preference where these points should go.

[jesperhodge]

I think these points should go here as long as they pertain just concretely to the project (see Adolfo's suggestion below, I am in favor of adding a more general doc or ADR for React best practices. We also have an internal wiki document on React best practices that is basically empty right now.) But I removed any design decisions here except for the point of not having non-hooks in hooks files, and the reason for that is that it is based on community standards and not on a subjective design decision.

[bszabo]

Design or coding question?

[jesperhodge]

Just for throwing out ideas where to put stuff if we don't have all our functions in the hooks file. Maybe we should not have this in here as it's not an important point?

[jesperhodge]

I removed this point

[bszabo]

Is the broader issue here that good stewardship calls for all test-related logic to live in test files, and for production files to only have code that contributes to features? And that our use of re-importing is causing test logic to live in production files? I haven't encountered this pattern, but wonder whether it's actually a "design for testability" issue. What is it that the re-importing is enabling for tests? Is there an alternate design that keeps test logic in test files? (looks like you already answered this in the note that follows)

[jesperhodge]

Yeah there are a lot of reasons that have apparently led up to this happening. The main reason for these self-imports in our project was that if you have two functions A and B in the same file and you want to test one and mock the other out you have some problems in JS. But I have never seen this as an obstacle in other codebases, and it is so easy to just throw one of the functions into another file, which is fine from a design standpoint - considering that webpack will merge those two files together anyway, so it has no performance implications to have a bunch of files.

[bszabo]

Are you suggesting that configuring a test with a specific starting state ought to be done with the mutator returned by useState, and never by mocking useState itself? Or are you suggesting that a test should never be in the business of directly setting state as part of its setup? (This is a clarification question only).

[jesperhodge]

Yeah it's actually a whole topic and very interesting point, and there are many very interesting articles on the topic. I would love to talk about the way we test in React, but I think that is very out of scope here and a larger topic and more controversial, so I just added this note to explain why you can get around without needing this weird pattern. Generally the idea out there is that you don't test your React hooks and state directly because they are not pure functions. They are all about side effects that control the lifecycle of the React component. A state hook behaves differently in development, test and production environments starting from React 18, so it is not deterministic. The hook calling setState may work amazingly well but from calling it you cannot see what it does with the lifecycle of the component, so you often have false positives. You also may have false negatives because the lifecycle and state may be controlled by different elements that make it work nonetheless. Basically what we are testing are not state hooks, but the component state and lifecycle, and we need to make that behave like in reality. So what the recommended way of testing React state is is to test the whole component as your unit and test the behavior when the component renders, when it loads data, when a button is clicked and so on. So now what if you have a React hook that is built for many components to reuse? And we don't have many of those anyway. If the hook is just part of your one component, best to test it as part of that component. Otherwise, you would in theory build a mock/testing component and add the hook to that component; but react-testing-library has tools to do that for you so you don't have to worry about it actually.

[arbrandes]

For the record, I'm very much in favor of this ADR. But shouldn't we put it somewhere for general than frontend-lib-content-components? Like https://github.com/openedx/open-edx-proposals, for instance.

[jesperhodge]

For the record, I'm very much in favor of this ADR. But shouldn't we put it somewhere for general than frontend-lib-content-components? Like https://github.com/openedx/open-edx-proposals, for instance.

@arbrandes It's a good idea. I would check with my team what they think. However, a part of this seems very specific - something like "don't use self-imports any longer" only is relevant since it's part of this specific project, right? Maybe we can split out only these very specific points and have them here in the project.

The reason I came up with the first "Principles" ADR was because I was missing a common language and set of guidelines that we can use when we talk about code quality/design. It's nice to say a certain pattern is better because of higher readability, but if people don't agree that readability is desired, that's hard to argue. But this is probably not the place for this ADR at all.

So I could imagine drafting an OEP that very generally decides what we want to achieve when we talk about good code quality and design, like in the "principles" here. And I can imagine a second OEP defining our Best Practices for developing React in general (we already have an internal confluence page on that, but it seems pretty empty so far.)
And leaving the very specific points relating to this project in the ADR here.

[arbrandes]

@jesperhodge, there are ADRs in open-edx-proposals as well; usually ones that go into dirtier details than in the OEP itself. Take OEP-45, for instance, and its ADRs (there's a new one coming down the pipes).

So, for instance, we can have a best-practices OEP with the general ideas you put forward here, then a more specific ADR for React.

[jesperhodge]

I made a new commit with the following purpose: in the reducing-complexity ADR, I removed anything that is just preference or convention, like where to put hook files. What remain should only be steps that are justified and necessary for fixing concrete problems.

I will also remove the "General Principles" ADR to later put it somewhere else - probably have it as an OEP, like Adolfo suggested. It needs to be heavily changed and iterated on, anyway.

The remaining ADR on reducing-complexity, only solving certain concrete problems in this codebase, I think is ready to change from draft to normal PR. There are more points we can find to improve the code design of this project, maybe add to that ADR, but they can be added in a later PR.

Points we have decided upon when this ADR is approved and merged that are in conflict with what ADR-0005 says, we should probably remove from 0005 when this is done.

[jesperhodge]

We have decided to close this PR. The idea after discussing it with the team is that this is not something that belongs into an ADR, exactly. We have agreed that most of these discussions tie into testing in some way; and that we would rather like to discuss improving our testing strategy for this repository. These code quality improvements may result organically from improving our tests. On top of that, we have an open issue for improving folder structure, created by Connor. So basically we want to tackle the larger challenges in the repo first and go from there.