neurobagel / annotation_tool

https://annotate.neurobagel.org/
MIT License
3 stars 6 forks source link

Refactor (most) components to use props instead of direct store access #582

Open surchs opened 11 months ago

surchs commented 11 months ago

This is a bit of a 180 on what we had discussed previously - giving component direct store access instead of pushing props around. I think the reasoning behind that decision was good: make it super clear who has responsibility for the global state.

However, this came with two downsides:

  1. Testing components is a bit laborious because we need to mock a bunch of getters, actions, mutations, and state objects. This is more complex - but sometimes just a little - than mocking similar props and spying events. See also point 4 here: https://www.cypress.io/blog/2023/02/16/component-testing-next-js-with-cypress#recommendations
  2. The bigger issue is that direct store access makes components tightly coupled to a specific use case. In other words: they are not very reusable. If I have a generic table component that I could use to show columns or unique values in columns, the fact that it is using very specific getters internally locks me into on or the other use case.

I suggest we keep what's good about the current solution, but slowly remove / refactor what's bad about it.

The good (what we keep):

The bad (what we want to change):

Instead we should:

Given our very strong test suite, we should be able to approach this refactor step by step, e.g. by changing pages and components we are already working on for some other reason.

github-actions[bot] commented 8 months ago

We want to keep our issues up to date and active. This issue hasn't seen any activity in the last 75 days. We have applied the _flag:stale label to indicate that this issue should be reviewed again. When you review, please reread the spec and then apply one of these three options: