Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
surchs opened this issue Oct 24, 2023 · 1 comment
Open
Labels
flag:discuss Flag issue that needs to be discussed before it can be implemented. _flag:stale [BOT ONLY] Flag issue that hasn't been updated in a while and needs to be triaged again refactor Simplifying or restructuring existing code or documentation. someday Not a priority right now, but we want to keep this around to think or discuss more.

Comments

@surchs
Copy link
Contributor

surchs commented Oct 24, 2023

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):

  • all important state is responsibility of the global store
  • component state is the exception, and only for transient UI state (e.g. dropdown open vs collapsed ...)
  • reused logic lives in the store, in the form of getters

The bad (what we want to change):

  • every low level component directly accesses the store.

Instead we should:

  • pages communicate directly with the store (but still don't have any state of their own)
  • pages pass props to child components and listen to child events
  • grand-children components have to get their props from their parent components. To keep prop-drilling to a minimum, we try to avoid deep tree structures in the app (i.e. ideally no more than 2-3 levels below the page)

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.

@surchs surchs added type:maintenance flag:discuss Flag issue that needs to be discussed before it can be implemented. refactor Simplifying or restructuring existing code or documentation. labels Oct 24, 2023
@surchs surchs changed the title Refactor (most) components to use pros instead of direct store access Refactor (most) components to use props instead of direct store access Nov 2, 2023
Copy link

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:

  • prioritize: apply the flag:schedule label to suggest moving this issue into the backlog now
  • close: if the issue is no longer relevant, explain why (give others a chance to reply) and then close.
  • archive: sometimes an issue has important information or ideas but we won't work on it soon. In this case
    apply the someday label to show that this won't be prioritized. The stalebot will ignore issues with this
    label in the future. Use sparingly!

@github-actions github-actions bot added the _flag:stale [BOT ONLY] Flag issue that hasn't been updated in a while and needs to be triaged again label Jan 17, 2024
@surchs surchs added someday Not a priority right now, but we want to keep this around to think or discuss more. refactor Simplifying or restructuring existing code or documentation. and removed type:maintenance refactor Simplifying or restructuring existing code or documentation. labels Dec 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
flag:discuss Flag issue that needs to be discussed before it can be implemented. _flag:stale [BOT ONLY] Flag issue that hasn't been updated in a while and needs to be triaged again refactor Simplifying or restructuring existing code or documentation. someday Not a priority right now, but we want to keep this around to think or discuss more.
Projects
Status: No status
Development

No branches or pull requests

1 participant