-
Notifications
You must be signed in to change notification settings - Fork 31
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
Restructure redux state to better reflect component state, validation and repeating groups #345
Comments
Update: This is kind-of being implemented in the layout expressions project. In this project, we're implementing ways to find a nearby component instance in relation to the current component, so implementing it something like that without tooling would have caused considerable headache. As of writing this comment, the layout expressions branch includes tooling for generating such a hierarchical representation of the layout. TL;DR: Consider the layout expressions project to be blocking this issue - it will quite possibly be easier to implement after that is merged into main. |
An update, yet again! 👋 I looked trough the documentation yesterday, and while reading about why we have a normalized redux store (which, in a way, is the problem we're trying to solve here), it turns out one of the challenges that solves is useless re-rendering caused by nested state. It might look like nesting state in redux is hard to get right. When I looked around for solutions, I found RecoilJS, developed by Meta/Facebook (just like React), and it looks like a more flexible (and at the same time simpler) way to share state in React - I really like the idea. I think we could perhaps build on RecoilJS to achieve this goal. When watching the deep dive video about Recoil, I quickly recognized an overlapping design philosophy with what I've been thinking here. I've wanted to get rid of our way of mutating the component id to make it unique (adding repeating group row indices like Introducing RecoilJS also requires getting rid of Redux Saga, making all of this more of a major rewrite. Getting rid of sagas sounds like a good thing though, and I'd love to have all events automatically triggered whenever the state they depend on changes - but writing procedural-like code (for sagas) in a reactive environment has its own set of challenges (mainly minimizing re-renders). For an example of this type of architecture and its problems, see #576. The |
Also relevant here (if we're smart, we can also solve this while we're at it, if we rewrite to RecoilJS): |
I think it's time to close this issue now. Pretty much everything related to this has been rewritten in v4, and we don't even have Redux there any longer. So the essence of what we wanted here, even if it may not include every smallest part of it, has been resolved now. Closing an epic Epic. 🙏 |
Description
Today,
app-frontend-react
will fetch the layout(s) from the server and store them in theformLayout.layouts
redux state almost unchanged. When, for example, a form input is changed, this will update the state informData
, a different redux root state. This is also the case for other state directly tied to a specific component, such as:formValidations.validations
stateformDynamics.conditionalRendering
and effects stored informLayout.uiConfig.hiddenFields
FileUpload
andFileUploadWithTag
), stored inattachments.attachments
FileUploadWithTag
), stored informLayout.uiConfig.fileUploadersWithTag
formLayout.uiConfig.repeatingGroups
Repeating groups contributes to making this especially difficult to work with, as we have to make sure all state is kept in sync when adding/deleting rows to repeating groups; i.e. that the validation state, attachments, etc. is updated to remove the state belonging to a row that was just deleted, and that subsequent rows gets "moved up" to replace the removed row.
With this architecture it means we have to write a lot of code to move stuff around, which sometimes might be finicky and might lead to bugs and unexpected behaviour (such as
FileUpload
not being supported inside repeating groups, some validations not working for DatePicker when in repeating groups or some validations not working in combination with conditional rendering when in repeating groups). It also means that implementing fixes for these require lots of code to move things around, which leads to large PRs for seemingly minor features.I propose to invert the way we use the Redux store to keep state about components, rewriting this to work in a way where all the state tied to a component is stored alongside the component itself.
Additional Information
Redux with nested state and dynamically adding reducers:
Relevant/related issues
Analysis
A proposed solution is to use nested slices to construct a hierarchy from the layout definition, along with mixing in re-usable reducers for common functionality (such as form data, validations, etc).
An example of such a hierarchical state (somewhat abbreviated, with explaining comments):
This solution should also handle
multiPage
groups, in a way such that the functionality becomes transparent to code using the group state.The point of
nodeId
here is to have an internal reference to each component (called node here, as a component can appear multiple times inside a repeating group). Updating the formData for a node can be as easy as dispatching an event with payload{ nodeId: 5, newFormData: 'hello world' }
. Each node will have its own reducer, and will only reduce actions specifying its ownnodeId
. Gaps in the sequence ofnodeId
s are allowed (as rows can be deleted and thus nodes will be removed), and care should be taken to ensure we don't write code that attempts to update/change thenodeId
for a given component instance once first constructed.Alternatives
Libraries like redux-orm might help keep relational data in sync, but it's quite possible it won't work directly with the state we have now.
Conclusion
This might be doable, but we'll need to move forwards with a proof-of-concept before committing to this approach. It WILL be a fundamental change.
The text was updated successfully, but these errors were encountered: