Open Mohammer5 opened 4 years ago
Offtopic: Thinking about it, we could also implement some kind of link middle-/afterware like apollo-link-state
for global state in apps. We could use redux internally for this, so it can be used for UI state (and for UI state only), like loading states with the capability to extend the allowed usage, just like apollo-link-state
does), at least it would give us the benefit of using useSelector
.
Ismay brought up a very good argument for using redux over context for this (which currently is the proper solution for global state in app-shell apps not using redux):
Actually if you are creating a single global state, redux is a better option, because it has well thought out optimizations in react-redux. When you change the state of your single store every single component connected to that context will re-render which is horribly unperformant. React redux handles this by not actually using the store state as the context value, but instead uses an event emmiter to communicate to connected components. They have written lots about it, which you will have to Google yourself as I’m eating dessert 😜. But point is react context is bad for single global state, but not bad if you bread your state in to lots of contexts so you don’t need to worry that any change affects all context consumers (from here)
apollo has this baked in as well, but I think for us, using redux is significantly easier that writing it ourselves.
When you change the state of your single store every single component connected to that context will re-render which is horribly unperformant.
This is only the case if you store the state as the context. We do not do this, nor should we ever do it. Each Query or Mutation (eventually subscription) currently has its own state and only rerenders itself, nothing else, when necessary. Eventually this might come from an event emitter from a centralized store (we'll need to do this when we add caching anyway)
I think providing an "out of the box" global state option might be worth considering, but I consider it distinct from loading states which are baked into the data engine runtime
This is only the case if you store the state as the context. We do not do this, nor should we ever do it.
I'm not talking about what the data provider does, but what apps have to do if they need global state.
EDIT: What I'm trying to say is: The data provider should provide a way to leverage on its state mechanism so the app doesn't have to use context
but I consider it distinct from loading states which are baked into the data engine runtime
Which is why I labelled it as "offtopic", I could create another issue, but think it's even less relevant than the current one
Hey @Mohammer5 - raising this again so it doesn't get lost.
As a quick question, would an implementation of optimistic updates solve your use-case? Basically, when mutating from somewhere else in the application any other queries for that object would get a "placeholder" value even while the HTTP request is in-flight. It can then be rolled back if the request fails, and replaced with the real thing if it succeeds.
I think this is a simpler API than a more generic state system or namespaced loading/error states, but obviously comes with some additional challenges to ensure we have a properly-normalized and indexed local cache.
I implemented a very rudimentary version of this with an EventEmitter here (emit) and here (subscribe) which works well but for a quite limited use-case.
Would optimistic updates be a possible way to address your use-case or do you think there would be requirements either for more "ad-hoc" namespaced loading states or for true propagation of "loading" rather than "stubbed" optimistic values?
I had some time to think about this. I think it wasn't so much about loading states themselves, rather about having shared client state between routes and/or components.
I can give you a simple example: Let's say one route displays an input field (e. g. for search) and the other route wants to display the same field, just somewhere different in the ui and the app wants to retain the value while changing routes.
Here's a second example: The import export app polls the current status of an import job and we want it to keep the import result (once the polling is done) so you could go back there after navigating to a different route. That's how the new v35 version works right now (with context)
Here are my current options with application state, each with good and bad sides:
This is obviously the most primitive choice but still easiest and pure-react-way choice. It comes with some downsides such as when having them across multiple depths in the react tree, it becomes very hard to know where they are if you don't know it already. It doesn't have many performance optimizations when dealing with large & deep react trees, one of the main reasons why so many people switches form context to redux. So normally you want to have the app's state on the root level.
This is not a bad idea in terms of pure & maintainable client side data. I think it won't be hard to implement either (looping through a set of entries when a query or mutation should be executed and let the client side resolver handle everything when it should be used). It would require some form of client side caching (like apollo's InMemoryCache
)
This would require the app to either initialize the data engine itself (apollo-like) or the engine offers an api to add client side resolvers during the runtime (engine.addResolver
?).
I guess this would be a MUCH better implementation than what I've proposed above and one of the main ideas why I think this could be a very good way of handling client side state is because there's only a single source of truth (the engine) for dynamic data.
What I don't like about the two solutions is that they're both quite imperative. I have to tell the state how to change instead of what to do, which is where redux shines. It's more very declarative and - once you understand the concept - very easy to extend or maintain. The react-redux
library has performance optimizations.
I still like the idea of the engine being the single source of truth though, so I'm wondering if we couldn't combine those two concepts:
@client
"directive"Obviously we couldn't leverage on the react-redux
lib in the pure engine
, but we'd still have a declarative mechanism for managing client side data while using the engine as a single source of truth. This could go as far as using redux-thunk, offering people to query third party endpoints (I remember that some people requested this) without the engine having to care about it and still being the single source of truth.
This could live side by side with optimistic updates, I assume that it could be a very complicated implementation though because you want every useDataQuery
to update that uses the mutated data source instead of imperatively update all queries that exist, which would introduce coupled code..
I was wondering about if we couldn't just use apollo directly and have a set of client side resolvers that do their "magic", basically an apollo server on the client side. But I don't know if there's a way to "catch" unknown query types because we obviously don't want to write the whole dhis2 schema as graphql schema.
I'm currently tackling something similar for the scheduler. My personal preference would be to stick to the primitive approach (React context) with a form of caching, if possible and maintainable. I'll see if I can make that work with the scheduler, will report back on how that goes.
The reason I'm advocating for redux instead of context is that redux's paradigm is declarative and intention-driven while context is imperative and state-driven.
As developer I find it much easier and safer to express my intention as code and then being able to reason about code quite comfortably instead of having to understand / know where state is going to be mutated.
Also using context in performance critical situations needs more adjustments on the app's side, which either complicates the app or leads the developer to using a different piece of technology. And I'd prefer consistence yet simple solutions.
I keep thinking about this and I want to add two more points to this discussion:
Although we can solve everything with context in terms of handling ui state / local state, I still favor the idea of a single source of truth (the data engine in this case). Our primary target is react, but the engine has been extracted on purpose, so in theory it should work in many contexts. If we agree on having a single source of truth, then context is already out of the game because it caters exclusively to react apps.
Imperatively/directly manipulating state can be seen as an action inside our business logic, while in reality there are two actions:
This distinction is not codified though, so as a developer you have to make assumptions why state is being manipulated.
Redux does make us codifying that distinction, which opens up some possibilities we didn't have before. Not code-wise but of cognitive nature as the developer has to make fewer assumptions, has to keep less code in mind and can therefore create more complex scenarios while the extra complexity is "hidden"/"covered" by redux.
I'm not trying to advocate for redux specifically, we could achieve the same with React's context (even with the same performance), but it'd require us to implement some kind of library (or workspace in this repo), increase the maintenance burden and on-boarding time. Many developers are familiar with redux on the other hand.
I still think this should be baked into to data engine though.
@ismay had a good remark:
. . . after removing redux from the scheduler I have to say it's been great . . . from a practical standpoint I haven't encountered any issues so far . . . the scheduler is also small, so maybe scale plays a role as well
My response to that is: if we build state management into the engine, we could think about simpler patterns for now until we see that it doesn't scale
Actually I agree with @ismay there. I'd be fine with starting more low level and see how it scales. If we don't need complex ui state, then it's probably even better not using redux.
Still would like to have a single source of truth. But we could defer this discussion and see how it plays out just using context for now. Maybe I'm trying to optimize something that doesn't need optimization here.
Hey guys! Thanks for the great discussion, lots of good points! I’ll respond in more detail shortly, in a marathon meeting today. I’m definitely in favor of a simple, lightweight solution first and designing for declarative single-source-of-truth access.
One detail thing I’ll add quickly is that I think I’d advocate for a separate “state” service within app-runtime, rather than commingling with data engine - basically an implementation detail, but I think keeping them separate is clean
I think the important things to define here are:
My initial thought these are not fully fleshed out, definitely need to discuss as a team so feel free to disagree!
In terms of requirements and simplifying assumptions the ones I see are :
In terms of API, I think I'd be in favor of something simple like the below but without reducers, action creators, dispatch, etc. - instead we could just expose two hooks :
const value = useGlobalState(key | selector)
- rerender with changes to either state[key]
or selector(state)
const update = useGlobalStateSetter(key | setter)
- return a function which is either value => state[key] = value
or value => state = setter(state, value)
The first version of each hook makes the assumption that state is just a dictionary of key-value pairs, the second is a bit more robust (Redux-like) but without the requirement of centralizing all action creators and reducers.
(In the example below I've gone the full redux clone route, but it would be easy to implement the above API in the same way).
I planned to just sketch this out and include it in-line, but ended up coding it as a sample app.
Note that here (as in the data service) I'm using React Context purely as a deep-vdom distribution mechanism - the value of the context itself never actually changes, so none of the context consumers re-render (avoiding the problems often associated with context for global state). Instead it just delivers a pointer to the current state, a dispatch function, and a couple helper functions for subscribing to changes to somewhere possibly very far down the React tree. This could technically also be done by passing store={store}
props down through a whole mess of children, but context allows intermediate components to be completely ignorant of global state, which makes refactoring 1000x better. (tangent - this actually reminds me a tiny bit of one of the benefits of algebraic effects, and also exception control flow in Javascript, which is that functions between a try
and a throw
don't need to care about exceptions at all. Really fun read!)
Definitely not saying this is the way to go, but wanted to show a proof-of-concept React Context-based approach with reducers and selectors. Here's the POC app I ended up with and below is the simple library. It doesn't support middleware or thunks, but does the right thing in terms of only re-rendering when a relevant slice of the state changes. It's basically a limited 60-line Redux clone, so at this point we might just want to use Redux (though no-deps has some appeal).
import React, { useEffect, useCallback, useContext, useState } from 'react'
const identity = state => state
export const createStore = (reducer = identity) => {
const subscriptions = new Set()
let state = reducer(undefined, undefined)
return {
getState: () => state,
subscribe: callback => {
subscriptions.add(callback)
},
unsubscribe: callback => {
subscriptions.delete(callback)
},
dispatch: action => {
state = reducer(state, action)
for (let callback of subscriptions) {
callback(state)
}
}
}
}
const GlobalStateContext = React.createContext(createStore())
const useGlobalStateStore = () => useContext(GlobalStateContext)
export const GlobalStateProvider = ({ store, children }) =>
<GlobalStateContext.Provider value={store}>
{children}
</GlobalStateContext.Provider>
export const useGlobalState = (selector = identity) => {
const store = useGlobalStateStore()
const [selectedState, setSelectedState] = useState(selector(store.getState()))
useEffect(() => {
// TODO: deep equality check, this only triggers an update on referential inequality
const callback = state => setSelectedState(selector(state))
store.subscribe(callback)
return () => store.unsubscribe(callback)
}, [store, /* ignore selector */]) /* eslint-disable-line react-hooks/exhaustive-deps */
return [selectedState, { dispatch: store.dispatch }]
}
export const useGlobalDispatch = () => {
return useGlobalStateStore().dispatch
}
export const useGlobalAction = actionCreator => {
const dispatch = useGlobalDispatch()
return useCallback((...args) => {
dispatch(actionCreator(...args))
}, [actionCreator, dispatch])
}
~yes @Mohammer5, the context object built in the provider can and should be split out into a non-react Store class and just wrapped by the provider ;-)~ EDIT: Went ahead and did this, made things WAY cleaner. Updating the above example
@ismay are you using React Context in the Scheduler for some global state or just local React state?
One thing I'm pretty sure about is that we tend to over-use Redux when we have it, when we should use local state for a LOT more things (particularly relating to UI). I actually think most of the examples @Mohammer5 used above should be accomplished without adding global new explicit state to the application. Instead, things like synchronizing refreshed data should be automatic when using the Data Engine, and things like shared loading/error state (in the Org Unit Tree, for example) should probably be handled with Suspense for Data Fetching and Error Boundaries, rather than adding something to the global state.
I was wondering about if we couldn't just use apollo directly and have a set of client side resolvers that do their "magic", basically an apollo server on the client side. But I don't know if there's a way to "catch" unknown query types because we obviously don't want to write the whole dhis2 schema as graphql schema.
@Mohammer5 I like where you're going here 🎉
I would love to use Apollo Client and GraphQL directly, but unfortunately until we get a GraphQL server into Core (I just presented a proposal to try for this in 2.36 but no promises...) we won't be able to. Handling GraphQL queries (Apollo Server) is quite heavy, and doesn't work in the javascript event loop. I once saw an implementation which ran the server in a Web Worker to process queries and translate them into REST calls, but I still don't think it's super performant and not worth the performance hit just for the developer experience improvement (nice as that would be)
The other (and probably bigger) major advantage of using GraphQL is the ability to combine queries into one network roundtrip (since GraphQL is sent in a POST body), so the plan is to push for that and get the Apollo Client experience as a huge bonus.
EDIT: Found one 3-year-old reference to client-side GraphQL, could be worth exploring but I'm very skeptical about the benefit-performance trade-off
Sorry for the super long comments! To reiterate these are the things I think we should iron out, then implement the simplest solution possible:
@Mohammer5 @ismay if it would be easier (or more fun) to brainstorm ideas and try to break things on a call let's set one up!
Unrelated to the suggestion, I'd like to mention this middleware: https://github.com/rt2zz/redux-persist
I've used it a few times already and it's quite nice. Not sure if this is a requirement for our apps, it does help with offline support, but I guess that we don't need to because we don't support mobile devices
@amcgee Your solution looks pretty good already!
I think I'd prefer a useStore(reducer: Function)
hook that could be used in the app.js
so there's no need for any extra Provider
component.
Components that need data from a 3rd party api could just use useEffect
to set the actions then, a simple version doesn't require thunks for now I suppose.
Ah, just another thing, I think actions should always look like this:
interface Action {
type: string,
payload: object,
}
@amcgee Your solution looks pretty good already!
It's a start!
I think I'd prefer a useStore(reducer: Function) hook that could be used in the app.js so there's no need for any extra Provider component.
To use context we need a Provider component unfortunately, because Context.Provider
needs to be rendered in the VDom
interface Action { type: string, payload: object, }
Yeah, that's a good call, 100% agree.
I'm a bit hesitant to go full-redux here because I think it ends up with tons of boilerplate when we are trying to stay simple. I realize the benefits of centralized state transition logic, but I don't know that we necessarily need the action-reducer pattern. (see : the usage example for my redux clone has more lines than the implementation 😉 )
I think typescript safety on centralized state would have a lot of benefits, but I'm not going to open that can of worms again here 😃 we'd implement this in typescript anyway here in app-runtime
, and it would be up to the application to use it or not for the structure of the state itself
I'm a bit hesitant to go full-redux here because I think it ends up with tons of boilerplate when we are trying to stay simple. I realize the benefits of centralized state transition logic, but I don't know that we necessarily need the action-reducer pattern. (see : the usage example for my redux clone has more lines than the implementation wink )
I can see the simplicity by not using a redux-like pattern in the beginning.
I think it'd suffice if we adopt the change
and batch
style that final-form has, sth like this:
// change a single value
const { change } = useGlobalState()
change(state => /* return updated state */)
// multiple changes
const { change, batch } = useGlobalState()
batch(() => {
change(state => /* return updated state 1 */)
change(state => /* return updated state 2 */)
change(state => /* return updated state 3 */)
})
// alternative to batch
change(
state => /* return updated state 1 */,
state => /* return updated state 2 */,
state => /* return updated state 3 */,
)
We could still use selectors
const { state } = useGlobalState()
const foo = getFoo(state) // getFoo = state => state.foo
const bar = getBar(state) // getBar = state => state.bar
The above example is imperative though, but adding reducer/dispatch functionality wouldn't be a breaking change
To use context we need a Provider component unfortunately
That can be done by the the DataProvider
. We'd just have to replace the used reducer from identity
to whatever is passed to useStore
..
Sorry for the super long comments!
PS: No worries! Sometimes I'm bombarding you guys with walls of texts
I think it'd suffice if we adopt the change and batch style that final-form has:
Hmm interstesting... I'm not familiar with final-form's change
/ batch
approach, is that to prevent multiple renders? If so I think React does that automatically by debouncing multiple state updates into one render, so we might not need to worry about it? We could add a simple debounce/batch in the store, if needed, without moving the imperative calls into the consumer components.
Here's the API I was thinking about for a "simpler" approach - under the hood it could actually even create actions and reducers, but I find this much simpler to reason about (and easier to code) than creating a reducer (with a switch case), action creator function, and action type constant for any state change:
Basically this takes advantage of first-class function primitives to do the same thing but without the repetition - the drawback is that you don't necessarily know the reducer from the get-go, but in practice if all the actions are colocated you can reason about them well enough. I'm using the term actions
here but that's probably too overloaded and we should use something different... (referring to this as the action-reducer
approach below)
// Action-reducers (could be all located centrally)
// These basically combine actionCreator and reducer
// We could enforce that these always global static consts (as we do with queries)
const arSetAnswer = answer => state => ({ ...state, answer })
const arDefineQuestion = question => state => ({ ...state, question })
// Consumer Components
const questionSelector = state => state.question
const Question = () => {
const [question] = useGlobalState(questionSelector)
return <span>
<strong>{question}</strong>
</span>
}
const Answer = () => {
const [answer] = useGlobalState(state => state.answer) // inline selectors work too
return <span>
<strong>{answer}</strong>
</span>
}
const SolveButton = () => {
const defineQuestion = useGlobalAction(arDefineQuestion)
return <button onClick={() => defineQuestion('What is the meaning of life, the universe, and everything?')}>What is the question?</button>
}
const RandomizeButton = () => {
const setAnswer = useGlobalAction(arSetAnswer)
return <button onClick={() => setAnswer(Math.floor(Math.random() * 10000))}>Randomize answer</button>
}
// App
// In this case, <GlobalStateProvider> is rendered by the App Shell, so the App doesn't need it.
export const App = () => <>
<Question />
<Answer />
<SolveButton />
<RandomizeButton />
</>
The entire state transition schema is those first two lines, compared to the same thing in Redux style which is 30+ lines and would usually be split across multiple files.
Obviously this is somewhat limiting, but it would actually be not so difficult to move to an action / reducer / dispatch pattern later (consumers shouldn't care if they're passing an action-creator or an action-reducer)
That can be done by the the DataProvider. We'd just have to replace the used reducer from identity to whatever is passed to useStore..
I think we want to keep the concerns of state and data management separate - each with their own context - at least for now. This still follows the principle of single source of truth (apparently I'm just retweeting Dan these days...).
When we have a <GlobalStateProvder>
we could render it automatically for the application in the runtime Provider (where we render the DataProvider and ContextProvider currently) but I see that this is exactly what you meant, since at that point the application would need a way to set the reducer... got it, yeah very true.
I think if we're providing a centralized reducer my preference would be actually to expose a separate <GlobalStateProvider>
and NOT include it automatically in the app-shell, since it's by definition application-specific so the identity reducer is totally useless out-of-the-box. If we went the "action-reducer" route (above) we should provide it out of the box, though, since there's nothing the application would need to define before
Application state shouldn't be shared by different library components by design, so we'd have to be careful about never accidentally including useGlobalState
in a library component.
Notably, though, complex components could render their own <GlobalStateProvider>
which would do the right thing and "constrain" state to that component.
is that to prevent multiple renders? If so I think React does that automatically by debouncing multiple state updates into one render, so we might not need to worry about it
That's exactly the purpose (probably to work with other technologies as well). I didn't know react does that actually :+1:
const arSetAnswer = answer => state => ({ ...state, answer })
const setAnswer = useGlobalAction(arSetAnswer)
const answerReducer = (state, answer) => ({ ...state, answer })
const setAnswer = useGlobalAction(answerReducer)
I think style-wise I'd prefer the second one, but that's probably just because I'm use to that "syntax". I'm not a big fan of currying / higher order functions anymore (there are few cases where it absolutely makes sense though), I prefer partial application when necessary and just pass in everything else at the same time when possible (which is possible most of the time)
since it's by definition application-specific so the identity reducer is totally useless out-of-the-box
Fair enough!
@Mohammer5 @ismay if it would be easier (or more fun) to brainstorm ideas and try to break things on a call let's set one up!
That sounds good, though at the moment I don't have the use cases clear yet. Personally I'd prefer to get the scheduler refactor close to done first.
At that point I should have a clear idea of any use cases that aren't supported yet (if any). Having the chat before that point would become too much of a philosophical exercise I think (at least from my perspective).
The point is to address the relevant use-cases, so that sounds good to me
Hi!
Due to a lack of activity on this issue over time (180 days) it seems to be stale. If still relevant, please provide information that moves it forward, e.g. additional information, a pull request with suggested changes, or a reason to keep it open.
Any activity will keep it open, otherwise it will be closed automatically in 30 days. Thanks! 🤖
This is currently not an issue, just creating for reference
Type
Feature request
Problem
Given there are two independent components. One for rendering a list of items (q-comp), the other one for mutating said list (m-comp; either by adding, changing or deleting items).
When the m-comp runs its mutation process, there is no way to know about this in the q-comp.
What I think the solution should cover
How I would do this in Apollo
Apollo offers the
apollo-link-state
middleware. It can be used to define client-side resolvers. I then define auseMutation
/useQuery
hooks which leverages on the one provided by apollo, but additionally sends mutations for the loading/error state. The following code is more or less pseudo code and doesn't work exactly like that, but it conveys the message.One important note:
Inside that hook, I define 3 queries for mutating the loading state, they all expect a
name
.I then use these to set the loading states for the provided name.
This way a component can add a namespace to the process it's executing
And a list file can explicitly state when it wants to access loading states.
I guess this way we could also implement a more explicit
refetchQueries
(a property on theuseMutation
config object). This doesn't really solve how to do explicitupdate
s ("optimistic update") though, I don't think we want to store all responses in the app-runtime's cache :