Open/Close AutoSearchInput dropdown with Redux state

[kumar303]

The AutoSearchInput component does setState({ autocompleteIsOpen: true | false }). It should use Redux state instead so that we can use the Redux development tool to work on this component. Beware that setState({ searchValue: '...' }) is still necessary because typing characters in a text box should not dispatch Redux actions.

For QA: Please check for regressions in the autcomplete functionality of the search box in the header

[willdurand]

To me, this logic (open/close) belongs to the component because it is a pure UI piece of information.

[tofumatt]

To me, this logic (open/close) belongs to the component because it is a pure UI piece of information.

I was thinking that when commenting/talking to @SeanPrashad but wasn't 100% sure and was admittedly half-occupied with other stuff 😅

Also given it already needs to use setState I think it's okay to leave out of Redux. The devtools are handy but we already don't store absolutely everything in Redux (usually in cases like this) so I think it's okay to not store it here either.

That said, I'm guilty of putting UI stuff in Redux too, so you could use my actions to argue against my words 🤷‍♂️

[willdurand]

The React devtools allow to (easily) toggle boolean attributes in the state (with a checkbox). I often have the React panel with the component I am working on selected.

[kumar303]

we already don't store absolutely everything in Redux

Correct. IMO that's a poor design choice that we inherited from the past.

this logic (open/close) belongs to the component because it is a pure UI piece of information.

Redux is designed precisely to store pure UI state.

Using setState() obviously works fine in the production app but it breaks time travel in the Redux developer tool. I don't think it's necessary to break time travel. We don't gain much by breaking time travel, so why break it?

Also, if we could finally move everything to Redux then things like error replays would be possible. All setState() changes will be completely lost though and these could be crucial to understanding what caused an error.

The strength of Redux only works when we use it everywhere.

Controlled form inputs are the only acceptable usage of setState(), mostly for performance reasons. These state changes would be lost in a Redux action replay but we typically have Redux state changes for the beginning and end of form input so it will be tolerable. We don't have Redux state changes for the beginning and end of the autocomplete dropdown which means replaying an error would not work in the current implementation.

[willdurand]

Redux is not a drop-in replacement for React state. And, Redux is useful to manage the app state, that is the data, not the UI stuff like "is this dropdown open/closed?" in my opinion. https://redux.js.org/faq/organizing-state#organizing-state-only-redux-state

[kumar303]

Before we converted the open/close overlay state change to Redux, it was not possible at all to develop one of those screens with Redux tools. Why break the tools? They are super helpful.

There are several open bugs filed by QA that no one else can reproduce. Imagine if we could just download a stack of actions and replay them? Life would be so good.

[kumar303]

Maybe I'm misunderstanding but the link you posted completely reinforces my point.

Is there value to you in being able to restore this state to a given point in time (ie, time travel debugging)?

YES; immense value. If we do not agree that this is valuable then we shouldn't be using Redux at all! Redux definitely adds overhead to the initial development process but once it's there you can edit and refactor it quite easily.

[willdurand]

You picked one point out of 5, the 4 others don't really go into your direction. Anyway, I understand what you mean (and did not see your previous comment before your last one). Yet, I don't think it is all or nothing.

I don't think we should be storing everything in redux and I don't think it is a "poor design choice" because only the component that renders the dropdown should know about whether it is open or not. I don't really want the whole app to get notified and potentially updated entirely because this dropdown switches from a closed to open state. That's my main concern.

Of course, I see the value in replaying a set of actions for bug fixes if we can achieve this somehow.

[kumar303]

You picked one point out of 5, the 4 others don't really go into your direction.

I'll clarify what I meant here. When considering the dropdown open/closed state I would indeed answer no to all of these questions except this one: Is there value to you in being able to restore this state to a given point in time? Answering yes to that one question validates using Redux. If we don't use Redux then it is not possible to restore state at any given time. Game over.

I don't really want the whole app to get notified and potentially updated entirely because this dropdown switches from a closed to open state.

That would just be a reducer bug like any other reducer bug. We have had success in preventing these bugs through code review. If the switch case in a reducer is checking for action types incorrectly it's usually pretty obvious.

By the way, tracking overlay open/close state is working very well. It executes all other reducers but they just ignore the action, similar to any other foreign action. For the first time ever I've been able to use Redux developer tools on overlay pages which is really helpful.

The Organizing State page in the Redux docs has some links to projects that aim to reduce the boilerplate around connecting local component state to Redux: redux-ui, redux-component, redux-react-local. I didn't know about these and took a quick look. I'm concerned about ID generation (mostly for SSR), how testable they are, and how we could use Flow to validate state but I like the idea of using an abstraction to make Redux-ification easier.

[willdurand]

The Organizing State page in the Redux docs has some links to projects that aim to reduce the boilerplate around connecting local component state to Redux: redux-ui, redux-component, redux-react-local. I didn't know about these and took a quick look. I'm concerned about ID generation (mostly for SSR), how testable they are, and how we could use Flow to validate state but I like the idea of using an abstraction to make Redux-ification easier.

All of them seem abandoned.

Let's stick to what you've done in the FormOverlay component then, and see how it goes. Reverting to good old local state won't be that complicated if it turns out that it does not work as well as expected.

[kumar303]

Now that we have withUIState() it is possible to convert AutoSearchInput and I tried it out. It's possible but it won't be helpful because the underlying Autosuggest component (a third party lib) uses setState() itself and defeats the purpose.

For the Redux developer tool, there is also a problem with how Autosuggest closes itself when it loses focus. Clicking away from the window and onto the Redux developer tool loses focus.

See https://github.com/mozilla/addons/issues/9914 for some more important conversions we can make.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. If you think this bug should stay open, please comment on the issue with further details. Thank you for your contributions.