Skip to content
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

Simplified typing #481

Open
christianalfoni opened this issue Jan 15, 2021 · 20 comments
Open

Simplified typing #481

christianalfoni opened this issue Jan 15, 2021 · 20 comments
Labels
enhancement New feature or request

Comments

@christianalfoni
Copy link
Member

christianalfoni commented Jan 15, 2021

Dear Overminders!

I have one thing I really want to do and that is to clean up and simplify the typing of Overmind. It was a long process figuring out how to do it and with acquired knowledge through all this work and later projects I would like to do this:

import { IContext } from 'overmind'

const config = { ... }

export interface Context extends IContext<{
  state: typeof config.state,
  actions: typeof config.actions,
  effects: typeof config.effects
}> {}

There will be no more implicit typing, the above is what everybody will need to do. This is how you type the different things:

const myAction = (context: Context) => {}

const myActionWithPayload = (context: Context, payload: string) => {}

// Now we support generics
const myActionWithPayload = <T extends TEvent>(context: Context, payload: T) => {}

const myOperator = map((context: Context, value: string) => {})

const state: State = {
  someDerived: derived((state: State, rootState: Context["state"]) => {})
}

And I believe that is it. This will need to be a breaking change to actually clean up the internal typing of Overmind. I was just wondering how you feel about this change, do you consider refactoring your apps to this typing too much work? 😄

@christianalfoni
Copy link
Member Author

christianalfoni commented Jan 17, 2021

As part of this change the operators are also extremely simplified, and best of all... you can just compose any action directly in as an operator.

const someAction = ({}: Context, value: string) => {}

const operator = pipe(
  someAction,
  wait(500)
)

It basically means operators finally are interoperable with plain actions. Which also means we do not need mutate, map and run anymore. You can just give it a plain action to do all those things. We rather have operators that goes beyond what a normal action can do. Like debounce, wait, fork, waitUntil etc.

This is such a huge improvement!

@suevalov
Copy link

Looks like a great improvement and it doesn't look like a lot of work to refactor our app.

@jmattheis
Copy link

Looks good, will there be errors if the signature of actions doesn't match (Context, Payload). f.ex. what would happen if an action similar to this is defined:

const myAction = (context: string) => {}

@christianalfoni
Copy link
Member Author

@jmattheis Good question!

Yes, when the actions are attached to Overmind it expects the signature of: ´(context: IContext)` , so you will get the error where you instantiate Overmind rather than where you define the action 😄

@christianalfoni
Copy link
Member Author

christianalfoni commented Jan 23, 2021

So a summary of what is being moved into next now is:

New typing, no more implicit typing:

import { IContext } from 'overmind'

const config = { ... }

export type Context = IContext<{
  state: typeof config.state,
  actions: typeof config.actions,
  effects: typeof config.effects
}>

Type actions:

const myAction = (context: Context) => {}

const myActionWithPayload = (context: Context, payload: string) => {}

Operator changes:

  • Now an operator is interoperable with actions, meaning you can just do:
const whatever = pipe(
  (context: Context, input: string) => input.toUpperCase()
)
  • Due to interoperable actions, the map, mutate and run operators has been removed. Just use plain action signature

  • forEach operators has also been removed as the typing never worked there (This might come back, have to research some more)

  • fork has new signature:

pipe(
  // "type" is a property on the object on the pipe
  fork('type', {
    FOO: (context: Context, event) => {
      event // Is now typed as FooEvent
    },
    BAR: (context: context, event) => {
      event // Is now typed as BarEvent
    } 
  })
)
  • parallel now actually returns an array to the pipe with the results of each operator

React hooks

  • useOvermind is removed in favour of explicit hooks (more efficient and cleaner)
  • useState hook now has an added signature
const ItemComponent = ({ id }) => {
  // The state you access within the callback is not tracked,
  // only the result and further access is tracked. Meaning this
  // component will only track the item, not changes to "items"
  const item = useAppState(state => state.items[id])
}

@ssijak
Copy link

ssijak commented Feb 4, 2021

@christianalfoni can you give full example of the new typings feature with a simple state and action? because when I try it like this typescript complains:

import { IContext } from 'overmind'
import { createActionsHook, createStateHook } from 'overmind-react'

export type StateType = {
  value: string | null
}

export const state: StateType =  {
  value: null
} 

const changeState = ({ state }: Context, value: any) => {
  state.value = value
}

export const config = {
  state,
  actions: {
    changeState
  },
}

export type Context = IContext<{
  state: typeof config.state,
  actions: typeof config.actions,
  effects: typeof config.effects
}>

export const useAppState = createStateHook<Context>();
export const useActions = createActionsHook<Context>();

'config' implicitly has type 'any' because it does not have a type annotation and is referenced directly or indirectly in its own initializer.ts(7022)
'state' is referenced directly or indirectly in its own type annotation.ts(2502)
'changeState' implicitly has type 'any' because it does not have a type annotation and is referenced directly or indirectly in its own initializer.ts(7022)

@christianalfoni
Copy link
Member Author

christianalfoni commented Feb 4, 2021

Hi @ssijak , when you write this in the same file you will get this error. If you rather do:

export type Context = IContext<{
  state: typeof state,
  actions: typeof actions,
  effects: typeof effects
}>

Then it works within the same file. typeof config.* only works across files. I guess this is some TypeScript bug, or just how it works. But typically you would not put it in the same file and there are no examples in the documentation. BUT! I totally see why you tried 😄

@ssijak
Copy link

ssijak commented Feb 4, 2021

Thanks for the quick answer! It works now, it does not complain. But I noticed some possible issues around actions. I am a relative typescript noob so maybe I'm talking nonsense but:

If I type action parameters as any like :

const changeState = ({ state }: Context, value: any) => {
  state.value = value
}

than c.changeState bellow is inferred to ()=>void type

export const useActions = createActionsHook<Context>();
const c = useActions()
c.changeState()

@christianalfoni
Copy link
Member Author

Thanks for testing! Doing any will also match void I guess, which is how actions identify "no payload". I would strongly argue that any should not be used as a typed payload, but it is confusing for sure 😄

any is typically used for unknown objects. Cause if it was like a string, number etc. you would actually type it. So doing {} instead will probably fix it, as it matches any object, but not void

But this is good for documentation for sure!

@ssijak
Copy link

ssijak commented Feb 7, 2021

Thanks. I encountered one more possible issue. I do not find OnInitialize exported from overmind in the next tag. Is it removed or?

edit: So I should just use onInitializeOvermind action?

@Michielnuyts
Copy link

@christianalfoni This is all coming in V28 right? Do you have a rough release date in mind?

@christianalfoni
Copy link
Member Author

There has been a little bit lack of time lately (Blame the baby mostly 😉 ), but also at codesandbox.io we are hiring a few people and it has also taken up a lot of my time.

But if you look at the issue list now I have marked some issues as NEXT RELEASE, those needs to be fixed. Good thing is that our codesandbox.io PR, codesandbox/codesandbox-client#5517, is now running green and should be merged shortly. Which was also a blocker for the next release.

So yeah, I hope to get some time soon to get through the pending issue and get that release out 😄

@mattiasjosefsson
Copy link

mattiasjosefsson commented Apr 7, 2021

@christianalfoni I wanted to try out these new typings with my current namespaced config where my state and actions are placed in a separate folder. However in order to access the new Context type it needs to be imported in the actions file which would cause a cyclic dependency, how would I go about avoiding this? I couldn't figure it out from your examples.

overmind/index.ts

import { IContext } from 'overmind'
import { namespaced } from 'overmind/config'
import * as status from './status' //<---- cyclic dependency

export const config = namespaced({
  status,
})

export type Context = IContext<{
  state: typeof config.state
  actions: typeof config.actions
}>

overmind/status/index.ts

import { state } from './state'
import * as actions from './actions'

export { state, actions }

overmind/status/actions.ts

import { Context } from '..' // <---- cyclic dependency

export const updateState = (context: Context, payload: string) => {}

Thanks for an awesome library!

@eirikhm
Copy link
Collaborator

eirikhm commented Apr 8, 2021

@mattiasjosefsson Move config into a separate file :-)

@mattiasjosefsson
Copy link

@eirikhm Aaah of course! Thanks!

@mattiasjosefsson
Copy link

@eirikhm Finally got around to test it but it doesn't help to move config into a separate file because actions are still going to be referenced by Context in overmind.ts

@eirikhm
Copy link
Collaborator

eirikhm commented Apr 20, 2021

@mattiasjosefsson something like this:

overmind/config.ts

import { merge, namespaced } from "overmind/config"

import { state } from "./state"
import * as actions from "./actions"
import * as effects from "./effects"

import * as status from "./status"

export const config = merge(
  {
    state,
    actions,
    effects,
  },
  namespaced({
    status,
  })
)

overmind/index.ts

import {
    createActionsHook,
    createStateHook,
    createEffectsHook,
  
  } from "overmind-react";
  import { IContext } from "overmind";
  
  import { config } from "./config";
  
  export type Context = IContext<{
    state: typeof config.state,
    actions: typeof config.actions,
    effects: typeof config.effects,
  }>
  

  export type RootState = typeof config.state;
  
  export const useAppState = createStateHook<Context>()
  export const useActions = createActionsHook<Context>()
  export const useEffects = createEffectsHook<Context>()

overmind/status/actions.ts

import { Context } from "../index"

export const updateState = (context: Context, payload: string) => {}

@mattiasjosefsson
Copy link

mattiasjosefsson commented Apr 27, 2021

@eirikhm Could it be just eslint that complains when it's not really an issue? It will complain with the rule import/no-cycle both for effects and actions that import the Context type.

actions.ts

import { Context } from '.' // Dependency cycle via ./config:4 eslint(import/no-cycle)
export const updateState = (context: Context, payload: string) => {}

config.ts

import { state } from './state'
import * as actions from './actions' // Dependency cycle via .:1 eslint(import/no-cycle)

export const config = {
  state,
  actions,
}

index.ts

import { createActionsHook, createStateHook } from 'overmind-react'
import { IContext } from 'overmind'

import { config } from './config' // Dependency cycle via ./actions:2 eslint(import/no-cycle)

export type Context = IContext<{
  state: typeof config.state
  actions: typeof config.actions
}>

export type RootState = typeof config.state

export const useAppState = createStateHook<Context>()
export const useActions = createActionsHook<Context>()

state.ts

export type TestState = {
  test: string
}
export const state: TestState = {
  test: '',
}

@avele
Copy link

avele commented May 21, 2021

@christianalfoni So there is no way to simplify writing actions? Duplicating export const everywhere doesn't feel good. Ideally, for me, it would be object actions with local type definitions.

const actions = {
  example (context: Context, params: { a: string, b: boolean }) {}
}

Right now if I try to write this way I get the error:

'actions' implicitly has type 'any' because it does not have a type annotation and is referenced directly or indirectly in its own initializer.ts(7022)

@Spring3
Copy link

Spring3 commented Sep 22, 2021

@mattiasjosefsson no, it is not an issue with eslint. It is definitely a cyclic reference. I faced the same problem.

It doesn't matter if some portion of code gets moved into a separate file, because Context type can be defined only when the full config object is merged together, and full config object includes actions, which require Context to type their args.

What you could try to do is declaring the module to define the Context type

store/index.ts

import type { IContext } from 'overmind';
import {
  createActionsHook, createEffectsHook, createStateHook, createReactionHook
} from 'overmind-react';
import { merge, namespaced } from 'overmind/config';

import * as settings from './settings';

const overmindStoreConfig = merge(
  {
    state: {},
    actions: {},
    effects: {}
  },
  namespaced({
    settings
  })
);

type OvermindConfig = IContext<typeof overmindStoreConfig>;

// here
declare module 'overmind' {
  interface Context extends OvermindConfig {}
}

const useOvermindActions = createActionsHook<OvermindConfig>();
const useOvermindState = createStateHook<OvermindConfig>();
const useOvermindEffects = createEffectsHook<OvermindConfig>();
const useOvermindReaction = createReactionHook<OvermindConfig>();

export {
  useOvermindEffects,
  useOvermindActions,
  useOvermindState,
  useOvermindReaction,
  overmindStoreConfig
};

settings/actions.ts

import type { IAction, Context } from 'overmind'; // and then import it like this
import type { State } from './state';

const updateSettings: IAction<State, void> = ({ state }: Context, settings) => {
  state.settings = {
    ...settings
  };
};

const onInitializeOvermind: IAction<Context, void> = ({ effects }, instance) => {
  instance.reaction(
    (state) => state.settings,
    (settings) => effects.storage.save(settings),
    { nested: true }
  );
};

export {
  updateSettings,
  onInitializeOvermind
};

@henri-hulski henri-hulski unpinned this issue Jul 10, 2023
@henri-hulski henri-hulski pinned this issue Jul 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

9 participants