Graylog2 / Graylog2/graylog2-server

Refactor: Overhaul Frontend Store architecture

Open
#6,477 0 comments 0 reactions 0 assignees View on GitHub
needs-discussion triaged
Dominant language
Java
Stars
8.1k
Forks
1.1k
Avg merge
1d 20h
Merged PRs (30d)
217

Description

## Current Problems

The current usage of Reflux for the React stores has the following disadvantages:

* Reflux is not maintained anymore
* We are stuck on an old version requiring reasonable effort (almost equal to switching to something else) to update
* Our current Reflux version aims at ES5 "classes" using mixins. Using it with ES6 classes/functional components required us to write our own `connect` helper
* Reflux stores itself are using pseudo-classes, making typing harder (no typing of `this` properties)
* It is not possible to parameterize stores. E.g. it is not possible to construct a store that only contains the stream rules of stream xy.
* Reflux does not offer using a central dispatcher. While this leads to independent stores (each store has its own dispatcher), it makes it impossible to batch state updates depending on each other
* When implementing complex data structures maintained in separate stores depending on each other this leads to cascades of updates, detrimental for performance and correctness. For the views we worked our way around this by implementing a hierarchy of stores and connecting promises with each other. This is working, but requires a higher amount of knowledge about the internal workings of Reflux and our stores than desired.
* Reusing stores across plugin boundaries required us to ensure that stores are not loaded multiple times, why we came up with the `(Combined|Store|Actions)Provider` workaround. Stores using this mechanism are imported with a `const FooStore = StoreProvider.get('Foo');` instead of a `import { FooStore } from 'stores/FooStore;`, breaking IDEs trying to resolve the file and breaking typing of stores. This also made testing harder and the created tests more brittle than necessary in a lot of cases because it was required to use a lot of `jest.mock()`ing to get the stores mocked properly before even importing the component.
* In the views, use of the `(Combined|Store|Actions)Provider` was experimentally and intentionally omitted and instead a wrapper function was used that registers each store/action upon export time. This allows the consumer of each store to import the store/actions just like any other dependency but still ensures it to be loaded only once (see `views/logic/singleton.js` for details)
* Reflux stores state management is not bound to the React tree. E.g. there is no way to "unmount" a store, forcing it to purge its state. It has to be implemented for each store independently and it has to be remembered to trigger this e.g. when a user logs out. Forgetting this leads to data leakage and/or incorrect behavior.
* We currently have no consistent error handling in stores. Stores explicitly call UI notifications(!) when encountering an error instead of passing error information and letting the UI decide what to do with it.

## Proposal

With hooks and the new style contexts, most of the functionality of Reflux is not necessary anymore and a lot of the aforementioned issues can be addressed. The recommendation is to gradually replace Reflux in a multi-step process:

1. Implement contexts for each store
2. Change consumers of stores to use the corresponding contexts instead
3. Iteratively replace store providers with something that is not using Reflux

A possible replacement for e.g. the `StreamsStore` could look like this:

```
[StreamsStoreContext.js]
type StreamsState = Array;
type StreamActions = {| refresh: () => Promise |};
const StreamsContext = React.createContext<{ state: StreamsState, error: StoreError }>();
const StreamActionsContext = React.createContext();

[consumer]
const StreamsList = () => {
const { refresh } = useContext(StreamActionsContext);
useEffect(() => refresh(), []);
return (

{({ state: streams, error }) => error ? : streams.map(stream => )}

);
};
```

A provider could implement passing actions and state on its own. A possible React-only implementation could look like this:

```
const fetchStreams = fetch('....');

const StreamsProvider = ({ children }) => {
const [streams, setStreams] = useState();
const [error, setError] = useState();
const refresh = () => fetchStreams().then(result => setStreams(result), e => setError(e));
useEffect(refresh, []);

return (


{children}


);
};
```

## Benefits

The immediate benefits of this approach are:

* Supplying different state in tests becomes as easy as using a constant `` wrapping the component(s) consuming the state
* Nothing is happening on import of the context provider but only when mounting it
* Context providers are not necessarily singletons (unlike Reflux stores), they can exist multiple times in different parts of the component hierarchy
* Context providers can also have parameters, scoping them to a subset of the data

Separating actions and state in two different context prevents unnecessary rerenders of consumers of just the actions. For more complex data structures, `useReducer` can be used in a provider component, dispatching the actions when called and modifying multiple data structures at the same time, leading to a single update. In addition, a single provider component can return multiple contexts, which also prevents unnecessary re-renders in contrast to a provider/store hierarchy where one update triggers another update for dependent states.

A sample for a more complex state provider that manages different entities using `useReducer` that prevents having to use a context provider hierarchy could look like this:

**Before**:
```
const SessionContextProvider = ({ children }) => {
const [session, setSession] = useState();
const isLoggedIn = session !== undefined;

const login = (_session) => setSession(_session);
const logout = () => setSession(undefined);

return (


{children}


);
};

const CurrentUserContextProvider = ({ children }) => {
const { session } = useContext(SessionContext);
const [currentUser, setCurrentUser] = useState();
useEffect(() => fetchCurrentUser(session).then(setCurrentUser), [session]);

return (

{children}

);
};
```

**After**:
```
const reducer = (state, action) => {
switch (action.type) {
case 'login':
const { session } = action;
const currentUser = await fetchCurrentUser(session);
return { session, isLoggedIn: session !== undefined, currentUser };
case 'logout':
return { isLoggedIn: false };
};

const SessionContextProvider = ({ children }) => {
const [{ session, isLoggedIn, currentUser }, dispatch] = useReducer(reducer, { isLoggedIn: false });

const login = (_session) => dispatch({ type: 'login', session: _session });
const logout = () => dispatch({ type: 'logout' });

return (



{children}



);
};
```

This makes deriving a new state from a given action atomic and reduces the amount of necessary re-renders before the new state settles.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.