Repository navigation
@cycle/state Reducer to expect defined state? #948
Description
Activity
I am actually currently working on the new
isolate+stateand yes this is a major pet peeve of mine. But I have no idea how to fix that, I would be very glad for ideasAwesome. Keep up the good work. The idea I have is to separate initialization of state from reducing of state. That way a reducer can assume initialized state.
It seems that the only place where initialization of state could occur in that case is at the call to
withState.So perhaps
withState(main, initial, name)
This raises the question whether it is appropriate for the state of a component in this paradigm to be initialized by its parent rather than by itself.
I would say that it is.
These reducers are defined by your app and composed in streams, so that you know the order of emissions of those reducers in the stream
reducer$that gets passed to the sink. So all you need to do is put the initialReducer in astartWithoperator, and then by definition all the other reducers can assume that theprev: Stateargument is defined.The problem is more about the types I think, how can we make the stateSource not return a possibly undefined state
Hmm, but does that happen? I don't recall having to do
state$.filter(isDefined). StateSource is a generic onS, withMemoryStream<S>These reducers are defined by your app and composed in streams, so that you know the order of emissions of those reducers in the stream
reducer$that gets passed to the sink. So all you need to do is put the initialReducer in astartWithoperator, and then by definition all the other reducers can assume that theprev: Stateargument is defined.That all pertains to runtime. This issue is regarding the TypeScript types.
The problem is more about the types I think, how can we make the stateSource not return a possibly undefined state
I'm not sure what the challenge in that is. If an initialized state is required in
withStatethen it can be assumed that the state source will always be defined.Hmm, but does that happen? I don't recall having to do
state$.filter(isDefined). StateSource is a generic onS, withMemoryStream<S>Perhaps
strictFunctionTypes: falseallows that?If an initialized state is required in withState
That's not going to happen for a tiny issue such as this one. Initial state is application logic, so it belongs inside
main, not outside of it. Plus, it would be a breaking change, and we want to avoid these.I use this signature for state reducer:
type Reducer<T> = (state: T) => TI suggest a new constructor
withInitialStatesomething like this:const withIntiailState = (main, initialState: T, name) => { const sandwiched = (sources) => { const mainSinks = main(sources) return { ...mainSinks, [name]: (mainSinks[name] as Stream<Reducer<T>>).startWith(_ => initailState) } } return withState(sandwiched, name) }
sandwichedwraps themaincomponent and in turn is wrapped withwithStateHmm, but does that happen? I don't recall having to do
state$.filter(isDefined). StateSource is a generic onS, withMemoryStream<S>Sure. If the logic is such that the initial
undefinedstate is not emitted then the typing of the source is correct and there's no problem with the source.The problem is in the sink type. The reducer demanded by the TypeScript type of the sink is the one that expects a possibly undefined state parameter. When a sink of type
Stream<(T: State) => State>is provided, it passes type checking only whenstrictFunctionTypes: false.When
strictFunctionTypes: true, the sink would have to be at leastStream<(T: State | undefined) => State>.That is why I claim that there's a type problem here. And the type problem exposes a missed opportunity of using type checking to make sure that reducers are always called with initialized state.
Currently, to conform with the sink types (with
strictFunctionTypes: true), the component must provide a reducer sink that is of typeStream<(T: State | undefined) => State>. And so I have to write handling ofundefinedor do some (undesired) type assertions.If an initialized state is required in withState
That's not going to happen for a tiny issue such as this one. Initial state is application logic, so it belongs inside
main, not outside of it. Plus, it would be a breaking change, and we want to avoid these.Initial state does feel like application logic, but also the mere having of state feels like application logic. And the mere having of state is removed outside the component to be the concern of
withState. So, along with it, should go state initialization.The concept that reduction and initialization of state is application logic makes sense to me. In most programs, state is initialized along with the possible type declaration of it, as soon as it exists. In current @cycle/state, state is practically initialized with the value
undefined.If it is so important to refrain from such a breaking change as the one I suggested, here is a proposal:
export function withState< So extends OSo<T, N>, Si extends OSi<T, N>, T = any, N extends string = 'state' >(main: MainFn<So, Si>, name?: N): MainWithState<So, Si, T, N> export function withState< So extends OSo<T, N>, Si extends OSi<T, N>, T = any, N extends string = 'state' >(main: MainFn<So, Si>, initializer?: () => T, name?: N): MainWithState<So, Si, T, N> export function withState< So extends OSo<T, N>, Si extends OSi<T, N>, T = any, N extends string = 'state' >(main: MainFn<So, Si>, b?: (() => T) | N, c?: N): MainWithState<So, Si, T, N> { const [initializer, name] = typeof b === 'function' ? [b, c ?? 'state' as N] : [null, b ?? 'state' as N] // ... }
The down-side of this proposal is that the initial state must be provided as a function that returns the initial state.
Another proposal is just a new function that provides a better alternative to
withState. PerhapswithStatecould be deprecated for a long time.withInitializedState(main, initialState, name) // alternative names withInitialState(main, initialState, name) stateful(main, initialState, name)
This is a TypeScript problem, and it shouldn't leak out to JavaScript users of Cycle.js.
Viable solutions are:
- Don't use
strictFunctionTypes - Handle
undefinedinputs in every single reducer you create (after all, you chosestrictFunctionTypes) - Figure out new typings (not new runtime APIs) that fix this issue
- Fix TypeScript to allow us to express these cases
- Fix stream types to allow describing the first emission type, versus the subsequent emission types, then make the state sink require a type for a Stream where the first emission is
(u: undefined) => Stateand the subsequent emissions areReducer- This idea wouldn't be immediately merged in, though, it would have to be thought out carefully and avoid other breaking changes
Inviable solutions are:
- Introduce
withInitialStateand deprecatewithState- This issue @cycle/state Reducer to expect defined state? #948 is not critical enough to require all other
@cycle/stateusers to migrate to a new API, issue @cycle/state Reducer to expect defined state? #948 is a corner case
- This issue @cycle/state Reducer to expect defined state? #948 is not critical enough to require all other
- Don't use
@mightyiam did you try how something like
((T: State) => State) | ((T: undefined) => State)would work?Fix stream types to allow describing the first emission type, versus the subsequent emission types, then make the state sink require a type for a Stream where the first emission is
(u: undefined) => Stateand the subsequent emissions areReducerThis would be ideal, but still a breaking change. Current implementation allows the usecase of passing a state initializer
(_: undefined) => State(set the whole state) or even(_: undefined) => undefined(reset the state) anytime. It's probably safe to assume this feature is being exploited.Inviable solutions are:
* Introduce `withInitialState` and deprecate `withState`withStateneed not be deprecated at all. The, if, introducedwithInitialState(possibly along with itsReducer<T>helper type) could coexist for users who prefer the stricter types. From an extra/different module even.I suggest a new constructor withInitialState
Setting an initial state is an app logic realm, should be done inside Main.
Viable solutions are:
- Don't use
strictFunctionTypes
There is an opportunity to provide more type safety to the users. "Don't use
strictFunctionTypes" is equivalent to "Cycle.js doesn't supportstrictFunctionTypes. The whole purpose of the optionstrictFunctionTypeseven existing is backward compatibility with code that was written before this particular strict-ness was available in TypeScript. Otherwise, TypeScript is constantly moving toward more strict (thorough/strong) type checking.- Handle
undefinedinputs in every single reducer you create (after all, you chosestrictFunctionTypes)
No one wants to do that, right?
- Figure out new typings (not new runtime APIs) that fix this issue
The only new strictly-type change that would fix this issue would be the one listed below, adding an additional type parameter to the stream library.
- Fix TypeScript to allow us to express these cases
I'm not sure what additional feature in TypeScript you imagine.
- Fix stream types to allow describing the first emission type, versus the subsequent emission types, then make the state sink require a type for a Stream where the first emission is
(u: undefined) => Stateand the subsequent emissions areReducer- This idea wouldn't be immediately merged in, though, it would have to be thought out carefully and avoid other breaking changes
Understood to be non-realistic.
Inviable solutions are:
-
Introduce
withInitialStateand deprecatewithState- This issue @cycle/state Reducer to expect defined state? #948 is not critical enough to require all other
@cycle/stateusers to migrate to a new API, issue @cycle/state Reducer to expect defined state? #948 is a corner case
- This issue @cycle/state Reducer to expect defined state? #948 is not critical enough to require all other
It should be considered a serious matter, unless Cycle.js wants to tell its users "don't use
strictFunctionTypes" or "we know these types are deficient — use assertions there".Here's another proposal:
withState(main, name) // current call, where name is optional withState(main, { initial: initialState, name }) // new overload
This could be a non-breaking change, right?
The sink type would depend on whether
initialStatewas provided. The sink type wheninitialStateis provided will be the one that does not includeundefined, because initial state was provided. TypeScript is able to do that. Here is simplified example code.- Don't use
@mightyiam Please quit your insistence on saying this is a serious matter and that the only solution is a new
withStateAPI.The viable versus inviable solutions I listed are my same arguments. If you use
strictFunctionTypesthen you handle all undefined inputs in reducers. If you don't use it, you don't need to handle it. Cycle.js doesn't have perfect TypeScript types (there are also type assertion issues withcycle/isolate) because at some point we would have to choose between perfect TypeScript support or Cycle.js architecture properties (such as app logic only inmain), and these are sacrifices we don't need to do just because you opened this corner case issue.If you proceed to insist on your same arguments, I'll ignore you because I have to spend my time wisely.
Setting an initial state is an app logic realm, should be done inside Main.
Main having state could also be regarded as app logic. Consider...
const main = withState( (sources) => { const state$ = sources.state.stream // ... do your thing ... const stateReducer$: Stream<(s: State | undefined) => State | undefined> = ... return { state: stateReducer$ } } )
Then...
const main = withInitialState( (sources) => { const state$ = sources.state.stream // ... do your thing ... const stateReducer$: Stream<(s: State) => State> = ... return { state: stateReducer$ } }, initialState )
I don't understand where this having argument came from. The only way
mainhas state is by receiving an input which is a Cycle.js source. Somainreceives state management utilities, it doesn't "define the having" of such utilities. And more importantly: the way it does that is the same treatment for DOM, HTTP, and other channels. So if we put "definition of having state" insidemain, we'd have to put all the other "definitions of havings" insidemainas well. Which would undermine the Cycle.js architecture that aims to allow mocking the channels for testing, or swapping the channels for different implementations of the effects.Setting an initial state is an app logic realm, should be done inside Main.
Main having state could also be regarded as app logic. Consider...
const main = withState( (sources) => { const state$ = sources.state.stream // ... do your thing ... const stateReducer$: Stream<(s: State | undefined) => State | undefined> = ... return { state: stateReducer$ } } )
Then...
const main = withInitialState( (sources) => { const state$ = sources.state.stream // ... do your thing ... const stateReducer$: Stream<(s: State) => State> = ... return { state: stateReducer$ } }, initialState )
There's probably also a misunderstanding. We typically say that
mainis the inner function (the anonymous arrow function), not the outer one. The outer function also doesn't define the "having of initial state", because the outer function wasn't written by scratch, it was generated by the utility function.If you proceed to insist on your same arguments, I'll ignore you because I have to spend my time wisely.
I have found that it doesn't make sense to provide initial state in the call to
withStatebecause initial state may depend on emits from the component's own sources.The type of the sink is still an inconvenience, but now I can't imagine a reasonable change in the API.
How about this: if a stream is provided as the sink, then it reduces a possibly undefined state. If an object with initial state and a reducer stream, then the reducer stream will reduce from defined state. The type of the state sink would be something like this:
// Current API type A<T> = Observable<(s: T | undefined) => T> // Additional API type B<T> = { initial: T reducer$: Observable<(s: T) => T> } type StateSink<T> = A<T> | B<T>
This could be backward compatible and provide a means to initialize state on component initialization (which makes sense to me) and prevents ever having
undefinedin a reducer.Initial state occurs in the component and not outside of it.
Also, it guarantees that a component has an initial state emit from its state source stream, which could be considered a benefit, because it could help prevent the no-UI-because-one-of-the-components-did-not-emit-on-DOM situation.
This just probably need to be set to
type Reducer<T> = (state: T) => Tif one needsundefinedthing just useReducer<State | undefined>Sinks in Cycle.js are only streams.
Sinks in Cycle.js are only streams.
Is that open for discussion?
As a solution to this corner case TypeScript bug? I don't think so. Small and specific type-level bugs should not require runtime-level shoehorn solutions that make exceptions to the app architecture.
Every usage of
@cycle/statein TypeScript will experience this issue, right? Does this qualify as a corner case?The possibility of this issue being solved somehow in TypeScript itself or in the types of the various stream/observable libraries is practically zero, right? If so, and if this type safety issue is not solved by making a runtime adjustment, then it's pretty much given up on, isn't it?
I refer again to #948 (comment)
Every usage of @cycle/state in TypeScript will experience this issue, right?
Don't enable
strictFunctionTypes(I said this already)
cyclejs/state/src/types.ts
Line 2 in 313762d
Is this exported
Reducermeant to be imported and used in apps? I find that in my app I would like to have a reducer type where state is always defined:Could we have a separate API for instantiating state, somehow, so that a
Reducercould assume positive existence of state?