Proposal: Use React context instead of prop drilling

[tortmayr]

What it does

• Refactor app component logic into a context provider
• Wrap the root component into the newly created MemoryAppProvider. This way child components can access the central state via context API and props drilling is no longer required.
• Refactor child components to access the central state info via context instead of props.

How to test

Full feature testing of the webview is required to ensure that everything works as before.

Review checklist

• as an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

• as a reviewer, I agree to behave in accordance with the review guidelines

[planger]

Thank you @tortmayr! I like the usage of React context for this project because all React components of the memory view share the same props/state to a very large part anyway. By centralizing it, we avoid prop drilling and the need for redundantly adding additional properties, e.g. for new settings.

In general of course the use of context vs props is a compomise between encapsulation of components and duplication. However, as the React components of the memory view share large parts of state and props anyway, I lean towards using a React context here.

@colin-grant-work @jreineckearm What do you think?

Of course this change is pretty strong conflict magnet, so we probably should decide soon whether we want to merge this change and when, to avoid conflicts in changes that are currently in progress.

Thank you!

[tortmayr]

In general of course the use of context vs props is a compomise between encapsulation of components and duplication. However, as the React components of the memory view share large parts of state and props anyway, I lean towards using a React context here.

I agree. Regarding encapsulation: context sharing has to be explicitly enabled on a per-component basis. (Either by assigning the contextType property or the useContext hook). Currently only the options and the table widget use the shared context. Here as you already mentioned it makes sense because the share a very large part of the state anyways. If only a minor subset is required we can still fallback to props drilling to not expose the full state to the component. (e.g. How it is done for the MoreMemorySelect widget)

[colin-grant-work]

I think I'm ambivalent towards these changes, with a few thoughts that lead me to no very strong conclusion:

• The current prop drilling is a bit annoying.
• This only saves us one level of drilling.
• This adds one additional layer of React components (in that sense, it saves us two levels of prop drilling, since it introduces another level through which we could drill).
• It adds an entire React mechanism to the logic - now things flow from two sources.
• @planger's concerns about encapsulation aren't without merit. I'm on record advocating in general for more explicit declaration of dependencies.
• The degree to which this breaks encapsulation seems just right: the options & table widgets are intended to be used together and synched to the same state: Context expresses that more clearly than replication of props.

I think I'd lean slightly against the change, on balance: without it, we don't have to worry about React Context at all as a mechanism; with it, we don't gain anything very concrete. I could be swayed, though, if there were other widgets that we think could / should exist alongside the table & options widgets and be synchronized to the same state.

[colin-grant-work]

Unrelated to your changes, but it seems we don't really ever use the endianness values we pass around.

[jreineckearm]

Just noticed the same. We have "Selectable word endianess" in #48 . But I was falsely assuming before that the word display was little endian already which seems to be the internal default. I've raised #88