Skip to content

RFC add store.withCurriedTypes helper #684

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

Closed
wants to merge 9 commits into from

Conversation

phryneas
Copy link
Member

@phryneas phryneas commented Aug 8, 2020

This might be a completely stupid idea. But it might also be very useful. I'm really not sure to be honest.

So what does this do?

It adds a store.withCurriedTypes method that takes either just one argument and returns it in a curried form, or a bunch of stuff in an object and returns it.

So, you'd call it

// yes that works, and it magically just returns the correct type
  export const useAppDispatch = store.withCurriedTypes(useDispatch);
  export const useAppSelector = store.withCurriedTypes(useSelector);

// or
  export const {
    useDispatch: useAppDispatch,
    useSelector: useAppSelector
  } = store.withCurriedTypes({
    useDispatch,
    useSelector
  })

// or even
  import * as RR from 'react-redux';
  export const {
    useDispatch,
    useSelector,
    connect,
    connectAdvanced
  } = store.withCurriedTypes(RR)

This might easily be extendable for createSelector and createAsyncThunk, but impossible for createSlice (once we add the callback notation with createAsyncThunk in).

Downside: we're replicating a good chunk of the types of external libraries with minor modifications as part of RTK that way.

And at the moment, this might do stuff if react-redux is not present, but I can catch that - I was just lazy for this experiment as of now.

As I said, I'm not sure if this is a good idea, but I guess it's definitely worth discussing :)

An alternative route were to add the suggestion to do

declare module 'redux' {
  export interface Dispatch<A extends Action = AnyAction> extends typeof store.dispatch {  }
}
declare module 'react-redux' {
  export interface DefaultRootState extends ReturnType<typeof store.getState> {  }
}

which would have the same effect on useDispatch, useSelector, connect etc. - but would augment the original types, which might run the risk of type poisoning these interfaces if these augmenting definitions were shipped, e.g. in monorepos.

@netlify
Copy link

netlify bot commented Aug 8, 2020

Deploy preview for redux-starter-kit-docs ready!

Built with commit 0da887c

https://deploy-preview-684--redux-starter-kit-docs.netlify.app

@codesandbox-ci
Copy link

codesandbox-ci bot commented Aug 8, 2020

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 0da887c:

Sandbox Source
Vanilla Configuration
Vanilla Typescript Configuration
rsk-github-issues-example Configuration

@phryneas
Copy link
Member Author

phryneas commented Aug 8, 2020

Well, it builds, but it has a bit of a bundling fuckup, so it's not testable right now :(

@msutkowski
Copy link
Member

Just wanted to drop this in here as a CSB preview - basic counter, thunk, asyncThunk, and extraArgument usage: https://codesandbox.io/s/latest-rtk-createthunk-curried-types-54cf4?file=/src/hooks.ts

I wanted to add some thoughts/arguments to the createThunk helper that is introduced in this. Being that we bake thunk in as the default, it makes a lot of sense to offer a helper to provide those types instead of showing AppThunk (currently only shown in the Advanced Tutorial, and redux docs) and exporting that. I've included an example in this CSB, and I think it makes a lot of sense for both readability and autocomplete functionality (even for JS users). My thought is that instead of all of these handwritten type helpers we currently have, the createThunk aligns with the overarching goal of this PR - to make types painless and help prevent common issues. At the end of the day, a user configures a store and then currys the types and that's it. If we show this in every tutorial/setup guide moving forward, we'll eliminate a lot of chatter about type resolution issues (imo).

// Just works, no additional ThunkAction types and less exposure to potentially
// confusing generics
export const multiplyByConditionally = createThunk(
  (amount: number) => (dispatch, getState) => {
    const {
      counter: { value }
    } = getState();
    if (value < 1000) {
      dispatch(actions.increment(value * amount));
    }
  }
);

/** Comparison without the createThunk helper

// needs to be a part of the default app setup
export type AppThunk = ThunkAction<void, RootState, unknown, AnyAction>

export const multiplyByConditionally = (
  amount: number,
): AppThunk => async (dispatch, getState) => {
    const { value } = getState();
    if (value < 1000) {
      dispatch(actions.increment(value * amount));
    }
}
*/

@phryneas
Copy link
Member Author

Alternative naming idea for the "core" helper of this PR:

type Store = typeof store;
const bindStoreTypes = initBindStoreTypes<Store>();
export const useAppDispatch = bindStoreTypes(useDispatch);

@markerikson
Copy link
Collaborator

Is this even still worth considering given our current TS usage guidance?

@phryneas
Copy link
Member Author

Difficult to say 🤔

It adds an extra step to setup, but createAsyncThunk gets significantly easier to use because you can skip most of the manual generics you would otherwise need if you wanted to access getStore (which right now always requires you going from no generics to all generics).

Also I do like the idea of a createThunk helper a lot, because the AppThunk we have in the docs at the moment does not really allow for a dynamic return type (it's only the same return type for all AppThunks). And it would allow us to document that you can also "just write normal thunks without cAT", which would maybe prevent some people from reaching to the wrong tool.

As for the R-R types: The hooks are probably fine with the guidance we have, but this also fixes the connect types. But it's a pain to keep it in sync with the DefinitelyTyped type definitions.

@phryneas
Copy link
Member Author

phryneas commented Jul 3, 2022

Gonna close that as this is not happening, but we should add some kind of currying helper at least to createAsyncThunk.

@phryneas phryneas closed this Jul 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants