Feature/improve documentation

[Dassderdie]

• Add Immutable types interface Immutability types #18
• Improve the READMEs on the root, shared and frontend Add documentation for state-mangement, actions, validation etc. #11
  • We will obviously have to regularly update it

[christianzoellner]

Hi,
as posmised, I read through your documentation and also successfully tried out your setup instructions.

Don't worry, my feedback isn't as detailed as it was for the customer handout. Also, I think the reviews from @ClFeSc and @Dassderdie already spotted a lot of the cases of unclear wording and other such details...

I only have a few high-level questions / remarks for you that I cannot link to specific pieces of text:

• In your discussion of immutable states, you argue that it's performant to have immutable states that can be compared or referenced. My (probably naive) question would be whether it might also be a problem, since that way every action + reducer operation requires copy/pasting all the data in the state, which in cases with a lot of actions and large states might be bad for performance.
• In your discussion of State management and synchronization it sounds like every client possesses the complete state of an exercise. However, as far as I understood it, clients may only have subsets of the actual state depending on the current viewport. Maybe this idea of clients subscribing to viewpoints is interesting enough to mention here on the starting page.
• For me it is confusing that the eslint folder in root does not contain another package of the monorepo. (I already don't like the .vscode and .github, but there at least the .-prefix makes it lcear that they are not code. My personally preferred repo style, which I know from Django, would be to have all non-code contents in the root folder (README, .gitignore, .vscode, dockerconfig, ...) and then one clearly recognizable subfolder with only contains code and code metadata files + maybe additional readmes. This style would be similar to the src folders you have in the packages which also separate non-code and code.)
  • (btw: this is my personal criticism and preference and meant as a discussion starter, not a hard constraint for your project)
• In your package subfolders there are inconsistent readmes:
  • backend has none at all
  • frontend has a rather detailed readme. However, in the architecture section some folders are explained which do not occur (e.g. src/app/pages) while others that do exist are not mentioned (e.g. src/assets)
  • shared has a concise readme with a seemingly complete architecture description. Also, there are links to the folders, which (at least partly) masked up for the paths being verbally incorrect (since they omit the src subfolder).
• Regarding subfolders: Your naming seems to be inconsistently switching between singular and plural (e.g. pages vs feature in frontend)

Also, two practical points:

• In my attempt with VS Code, the both debug configurations threw an error.
• Also, when I "Start all" and open the frontend in my browser, the site did not permit the "adding patients" behavior from your demo. However, I saw usernames printed on the server console, so the test setup seems to function in principle.

The question for both: Is that intended behavior? Are the respective code fixes part of another branch?

Best regards,
Christian

[Dassderdie]

@christianzoellner

In your discussion of immutable states, you argue that it's performant to have immutable states that can be compared or referenced. My (probably naive) question would be whether it might also be a problem, since that way every action + reducer operation requires copy/pasting all the data in the state, which in cases with a lot of actions and large states might be bad for performance.

A reducer doesn't have to copy the whole state by value. Instead it should only recursively shallow copy all the objects that have been modified.

SHALLOW COPY
Shallow copy is a bit-wise copy of an object. A new object is created that has an exact copy of the values in the original object. If any of the fields of the object are references to other objects, just the reference addresses are copied i.e., only the memory address is copied.

source

Example:

const state = { 
    a: new VeryLargeObject(),
    b: {
        c: 1
    },
    d: {
        e: 2
    }
};

A reducer that increments state.b.c by 1 would look like this:

function incrementByOne(state) {
    return {
        a: state.a,
        b: {
            c: state.b.c + 1
        },
        d: state.d
    };
}

Neither state.a nor state.d would have to be copied by value. Only their references are copied. state.d.e would not even be read.

To improve the syntax we use immer as a wrapper for our reducers.

Immer can convert mutable actions to immutable ones. This improves the code of the reducer by a lot.

function incrementByOne(mutableDraftState) {
    mutableDraftState.b.c += 1
    return mutableDraftState; // this is converted to an immutable object again by immer
}

Performance problems could occur if one of the following is done (I haven't thought about this until now -> I'll add this to the documentation):

• a very large Primitive (a string that is a base64 encoded image) is added in a part of the state that is often modified (e.g. the root).
• Expensive search operations are done, that read from many objects. This performance problem can be solved by performing the read operations on the original state (e.g. via original()).

I'll look into the other points later.

[christianzoellner]

A reducer doesn't have to copy the whole state by value. Instead it should only recursively shallow copy all the objects that have been modified.

@Dassderdie thanks a lot for your very detailed response. I think this first sentence and the Idea that a state is structured hierachically so that whole "branches" of the data can be duplicated by a few shallow copies would have sufficed for me. But maybe you can persist this explaination somewhere, it might be useful for someone else.

[Dassderdie]

@christianzoellner

In your discussion of State management and synchronization it sounds like every client possesses the complete state of an exercise. However, as far as I understood it, clients may only have subsets of the actual state depending on the current viewport. Maybe this idea of clients subscribing to viewpoints is interesting enough to mention here on the starting page.

I didn't include this part in the documentation, because I'm not really sure about it yet.

The current concept encapsulates all the synchronisation stuff very nicely. If I add an action and a reducer I don't have to think about any conflicts or special cases that could occur.

If clients could have different states, a client must not propose an action whose reducer reads from a part of the state the client isn't subscribed to. Else the state is (permanently) out of sync.

@ClFeSc and I believe that we surpass the must-have and should-have requirements for the number of clients that can be connected to an exercise at the same time. This is partly based on my back-of-the-envelope calculation here. In addition it is not very likely that each client is sending a message every second. Short bursts of messages should only result in a temporary increase in the latency for the clients.

To validate these assumptions soon I created #25.

We could filter messages out that are not relevant for a client - but from what I know the overhead of sending a message is not a lot.

[Dassderdie]

I created #26 to track this discussion.

[Dassderdie]

@christianzoellner

For me it is confusing that the eslint folder in root does not contain another package of the monorepo. (I already don't like the .vscode and .github, but there at least the .-prefix makes it lcear that they are not code. My personally preferred repo style, which I know from Django, would be to have all non-code contents in the root folder (README, .gitignore, .vscode, dockerconfig, ...) and then one clearly recognizable subfolder with only contains code and code metadata files + maybe additional readmes. This style would be similar to the src folders you have in the packages which also separate non-code and code.)

I renamed eslint to .eslint. Other monorepos often have a projects or app folder where the actual packages go. I personally don't see the benefit of this because we only have three sub packages. Adding an additional folder would also make the paths longer. But I would be open to change it if someone from the team dislikes the current structure.

In your package subfolders there are inconsistent readmes:
backend has none at all

I won't add the readme in this branch. #27

frontend has a rather detailed readme. However, in the architecture section some folders are explained which do not occur (e.g. src/app/pages) while others that do exist are not mentioned (e.g. src/assets)

The other folders are described in the angular documentation. I added a link to the readme.

shared has a concise readme with a seemingly complete architecture description. Also, there are links to the folders, which (at least partly) masked up for the paths being verbally incorrect (since they omit the src subfolder).

I added the src.

Regarding subfolders: Your naming seems to be inconsistently switching between singular and plural (e.g. pages vs feature in frontend)

I renamed feature to features.

Also, two practical points:

In my attempt with VS Code, the both debug configurations threw an error.
Also, when I "Start all" and open the frontend in my browser, the site did not permit the "adding patients" behavior from your demo. However, I saw usernames printed on the server console, so the test setup seems to function in principle.
The question for both: Is that intended behavior? Are the respective code fixes part of another branch?

Both configurations and the Start all task should be working on this branch. At least they are on two computers of mine.
You can either create an issue in this repository with the errors or we find a time to look on it together via discord.

[Dassderdie]

This branch has been reviewed by two people. If you have any other suggestions/concerns/comments/improvements please create a new issue.

I'll merge this now.