mirror of
https://gerrit.wikimedia.org/r/mediawiki/extensions/Popups
synced 2024-11-16 20:18:00 +00:00
79 lines
3.3 KiB
Markdown
79 lines
3.3 KiB
Markdown
|
# 3. Keep enabled state only in preview reducer
|
||
|
|
|
||
|
|
Date: 14/12/2016
|
||
|
|
|
||
|
|
## Status
|
||
|
|
|
||
|
|
Accepted
|
||
|
|
|
||
|
|
## Context
|
||
|
|
|
||
|
|
Discussed by Sam Smith and Joaquin Oltra.
|
||
|
|
|
||
|
|
There is global state for determining if the previews are enabled or disabled.
|
||
|
|
It lives in the `preview` reducer as the `enabled` key.
|
||
|
|
|
||
|
|
As part of implementing the settings, it was noticed that:
|
||
|
|
* When the settings are saved in the UI dialog,
|
||
|
|
* And a `SETTINGS_CHANGE` action is triggered with the new `enabled` state,
|
||
|
|
* Then the settings reducer (`reducers/settings.js`) also needs to know about
|
||
|
|
the previous `enabled` state to determine if it:
|
||
|
|
* should ignore the change
|
||
|
|
* hide the UI
|
||
|
|
* or show the help if the user is disabling
|
||
|
|
|
||
|
|
Given the enabled state was kept in the `preview` reducer, there are several
|
||
|
|
options considered:
|
||
|
|
|
||
|
|
1. Add an `enabled` property to the `settings` reducer, duplicating that part
|
||
|
|
of the state in both `preview` and `settings` reducers.
|
||
|
|
* **Pros**
|
||
|
|
* `saveSettings` action creator remains synchronous
|
||
|
|
* `settings` reducer internally contains all it needs to act on actions
|
||
|
|
* **Cons**
|
||
|
|
* `enabled` state, action handling and state toggling, and tests are
|
||
|
|
duplicated in both `reducers/preview` and `reducers/settings`
|
||
|
|
* Maintenance overhead
|
||
|
|
* Confusion about which of both to use for taking decisions in other parts
|
||
|
|
of the source
|
||
|
|
* Risk of the `enabled` flags getting out of sync with future changes
|
||
|
|
2. Rely on UI dialog captured state to trigger `saveSettings` with the current
|
||
|
|
and new state.
|
||
|
|
* **Pros**
|
||
|
|
* `saveSettings` action creator remains synchronous
|
||
|
|
* `settings` reducer gets via action all it needs to act on actions
|
||
|
|
* **Cons**
|
||
|
|
* `enabled` state is duplicate in the `preview` reducer and in the created
|
||
|
|
settings dialog (either as a captured variable, or as DOM state)
|
||
|
|
* Confusion about which of both to use for taking decisions in other parts
|
||
|
|
of the source
|
||
|
|
* Risk of the `enabled` state on the UI getting out of sync with future
|
||
|
|
changes, and triggering stale state with the action resulting in bugs
|
||
|
|
3. Keep the `enabled` flag in the `preview` reducer as the canonical source of
|
||
|
|
`enabled` state. Convert `saveSettings` to a `Redux.Thunk`, to query global
|
||
|
|
state, and then dispatch current and next state in the action.
|
||
|
|
* **Pros**
|
||
|
|
* `enabled` state exists in just one place in the whole application. No
|
||
|
|
duplication
|
||
|
|
* `settings` reducer gets via action all it needs to act on actions
|
||
|
|
* **Cons**
|
||
|
|
* Confusion about the use of a `Redux.Thunk` on a synchronous action creator,
|
||
|
|
where in the docs they are used only for asynchronous action creators
|
||
|
|
|
||
|
|
## Decision
|
||
|
|
|
||
|
|
After code review and discussing the different options and trade-offs, the
|
||
|
|
implementation of **`3`** was chosen mainly because of the clarity that having one
|
||
|
|
piece of state just in one place brings for understanding the application, and
|
||
|
|
the benefits on maintainability.
|
||
|
|
|
||
|
|
Extensive documentation and tests have been written in the action creator and
|
||
|
|
in this document to explain the choice made, which should overcome the cons.
|
||
|
|
|
||
|
|
## Consequences
|
||
|
|
|
||
|
|
The `saveSettings` action creator is now a `Redux.Thunk`, which uses `getState`
|
||
|
|
to query the enabled state in the `preview` reducer, and adds it to the
|
||
|
|
`SETTINGS_CHANGE` action as `wasEnabled`. As such, the `settings` reducer can
|
||
|
|
act on `SETTINGS_CHANGE` to perform its business logic.
|