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

[Docs] Rework "Usage with TypeScript" page #4025

Closed
markerikson opened this issue Mar 4, 2021 · 7 comments
Closed

[Docs] Rework "Usage with TypeScript" page #4025

markerikson opened this issue Mar 4, 2021 · 7 comments
Labels

Comments

@markerikson
Copy link
Contributor

markerikson commented Mar 4, 2021

The current "Usage with TypeScript" page has multiple issues, and we should rework it. When that page was written in 2018-ish, I still didn't know TS yet. So, I couldn't offer any meaningful feedback on the contents. Now that I actually have a decent TS background, I can see that there's problems:

  • The page shows patterns like hand-written action creators and creating unions of action types, which are both patterns we now discourage ( https://phryneas.de/redux-typescript-no-discriminating-union )
  • It should show RTK as the default approach first, and hand-writing individual pieces as a fallback
  • We have mismatched information between the three different TS usage pages in our docs (Redux core, React-Redux, and RTK), and we should harmonize them to have the same advice in all places
  • We should better explain things like extracting the custom store Dispatch type to allow correctly dispatching thunks. (This is by far one of the most common questions I see lately, and I'm not sure how to easily address it through code, so we need to highlight the importance of this for thunk usage.)
  • Per Root state properties being typed as never with redux 4.0.5. #3690 (comment) , we don't mention the Reducer type
  • Along with all that, the CodeSandbox example link is apparently busted anyway.

and I'm sure there's some other improvements we could make as well.

The recently updated "TS Quick Start" page in the RTK docs would be a good starting point for "here's what to do":

https://redux-toolkit.js.org/tutorials/typescript

and then we can give more details for specific situations after that.

@ricokahler
Copy link

ricokahler commented Mar 14, 2021

Hey Mark, we discussed me possibly contributing here via Twitter. Here's some thoughts:

The page shows patterns like hand-written action creators and creating unions of action types, which are both patterns we now discourage ( https://phryneas.de/redux-typescript-no-discriminating-union )

  1. I think this pattern is mostly fine. It definitely has its downfalls but its appeal is simplicity (user-defined type guards are advanced TS usage by their docs). I think adding AnyAction to the union type of action may be sufficient in providing an accurate type that isn't lying. I can play with it and confirm.

It should show RTK as the default approach first, and hand-writing individual pieces as a fallback

  1. I think we should do a callout to RTK, put it first, and make it big/emphasized. I don't think we need to show any examples inline, just always point them to the appropriate project (helps with documentation entropy). Might help with these comments too Root state properties being typed as never with redux 4.0.5. #3690 (comment)

We have mismatched information between the three different TS usage pages in our docs (Redux core, React-Redux, and RTK), and we should harmonize them to have the same advice in all places

  1. Same as above, removing duplicated docs and pointing to the right projects should help with this. We can also do a final audit to ensure they don't conflict.

We should better explain things like extracting the custom store Dispatch type to allow correctly dispatching thunks. (This is by far one of the most common questions I see lately, and I'm not sure how to easily address it through code, so we need to highlight the importance of this for thunk usage.)

  1. I actually have an idea for this one. We could suggest augmenting the default types instead of re-exporting a function. This will make it so that you're importing the default useSelector etc and it ships with the user's types. Has some javascript benefits as well.

    Regardless, I think some explanation on why the type of Dispatch can vary (depending on the plugins) would help greatly.

Per #3690 (comment) , we don't mention the Reducer type

  1. Easy enough, this can be updated.

Along with all that, the CodeSandbox example link is apparently busted anyway.

  1. If you have the code available, I can attempt to fix it but tbh I did not notice that link. I think it should be more prominent if we want to keep it.

Random Notes:

  • "static typing" page on react-redux couldbe renamed to "Usage with Typescript" to be consistent with the other pages (though we should add a redirect to this new page)
  • we should create a new section in the main redux docs for typing dispatch given the common redux-thunk use case. We should expand on that dispatch types for react-redux and use dispatch. I like the idea of type augments. Redux-toolkit can point to both docs.
  • Random thought when reading through the docs: "As TS cannot combine two string literals (slice.name and the key of actionMap) into a new literal," — this is now possible with ts literal types
  • type predicate docs in TS point to a deprecated page now

@markerikson
Copy link
Contributor Author

@ricokahler I really do want to drop the "union of action types" thing. Per Lenz's article, it doesn't have any real benefit, and it's even more useless when you're writing code with RTK.

Agreed that we should rename the React-Redux TS page. I originally had thoughts of getting some Flow examples in there, but Flow is dead at this point.

Also agreed that we really need to emphasize how to type dispatch for use with thunks correctly.

Not sure I'm keen on the idea of augmented types, at least for now. I think Lenz has some opinions on that, though.

Paging @phryneas and @msutkowski for their thoughts here.

Tell you what, why don't you go ahead and try to put up a PR so we've got some concrete changes for discussion.

@ricokahler
Copy link

@ricokahler I really do want to drop the "union of action types" thing. Per Lenz's article, it doesn't have any real benefit, and it's even more useless when you're writing code with RTK.

I'm conflicted about it. @phryneas's article makes a lot of sense (and is great!) but the solution presented requires more code and loses the idiomatic redux that we've known for years. I actually do think it's possible to tell typescript that our action could potentially contain more than just the given union.

Here's what I came up with:

const initialState = { messages: [] }

export const SEND_MESSAGE = 'SEND_MESSAGE'
export const DELETE_MESSAGE = 'DELETE_MESSAGE'

interface ChatState { }

interface SendMessageAction { type: typeof SEND_MESSAGE, messages: string[]; }
interface DeleteMessageAction { type: typeof DELETE_MESSAGE }

type Actions = SendMessageAction | DeleteMessageAction;
type AnyAction = { type: any }
type OtherActions<T extends AnyAction> = { type: T extends T['type'] ? T : { type: unknown } }

export function chatReducer(
  state = initialState,
  action: Actions | OtherActions<Actions>
): ChatState {

  switch (action.type) {
    case SEND_MESSAGE:
      return {
        messages: [...state.messages, action.messages]
      }
    case DELETE_MESSAGE:
      return {
        messages: state.messages // etc etc
      }
    default:
      return state
  }
}

ts playground link

The trick here is type OtherActions<T extends AnyAction> = { type: T extends T['type'] ? T : { type: unknown } }.

Its type parameter accepts the union of all action types and uses a conditional type to assert that if the type doesn't conform, return { type: unknown }. This effectively creates a complement to the given union. If you don't add the default case, it errors.

image

I think there's some value in matching the switch style for the sake of matching all the historical material (or even just the rest of the docs) that exists out there. E.g. if someone new is coming to redux, most learning material will probably show the switch stuff.

I think the practical answer we can allude to would be "just use redux toolkit" but for the sake of the docs, we should show a type example as close to historical redux as possible. I really think discriminated unions are the best mechanism for this.


Not sure I'm keen on the idea of augmented types, at least for now. I think Lenz has some opinions on that, though.

I like the idea of type augments because it meets people where they're at. What's nice about it is that it brings the typescript usage closer to the non-typed usage (and vice versa e.g. javascript users can get type hints).

In general, I try to create type solutions that don't require any runtime support (as you can see from my previous recommendation). I do this for two reasons:

  1. it reduces documentation entropy. If your typescript usage matches your javascript usage, it is less confusing for typescript users to navigate the docs (and you can do cool things like write examples in only typescript and compile them to javascript).
  2. The typescript compiler is always evolving. If you can't type something now doesn't mean you can't type something in the future.

Let me know what you think!

I'd like to write the docs using the OtherActions type. If I get the greenlight, I'll get started.

@phryneas
Copy link
Member

Hmm.

Let's get the augmentation thing out of the way first.

I think for state it's amazing if you look at it in separation. And enormously dangerous if you look at it in real life.
If people start using that in libraries or library-style parts of monorepos, there is a good chance that these augmented types make it into projects where they don't belong and state structure would seriously become weird at that point.

Pretty much the same goes with augmenting the Dispatch type. While I would totally love to see that augmented dispatch type in the redux-thunk types get released and would enthusiastically use it in my own projects, it is a fact that that type augmentation would always be in place - no matter if the middleware is only installed (e.g. as a dependency of RTK) or actually instantiated (it can be disabled after all).

Getting back from that to the actual concern of rewriting the "static typing" page: I'm not sure we should actually add any new information on that.
People seem to skip all introduction and start wildly copy-pasting code. No matter how many warnings we attach that this is only meant for legacy projects and that we really recommend all TypeScript projects to use RTK as that is designed to work with TS: people will just start writing their JS code they learned from some crappy udemy course and adding types into it. Then they will get frustrated, start googling around wildly (still discarding the docs) and start using approaches like typesafe-actions, which were a great things back in 2018 but which I would not really recommend as the first choice nowadays.

My personal approach would honestly be to replace the whole "static typing" page with a section that emphazises that we really really really want people to use RTK. And then add a link to the github archive of that page so that people who need TS in a legacy project still can read up on it. But really just in the raw markdown form, to really empathize that this is part of the history of Redux and not how we suggest it to be done nowadays.

This might seem a bit drastic but I'm honestly getting frustrated by the amount of people ignoring all the blinking signs and doing themselves the disservice of learning only vanilla redux.

@ricokahler
Copy link

I think for state it's amazing if you look at it in separation. And enormously dangerous if you look at it in real life.
If people start using that in libraries or library-style parts of monorepos, there is a good chance that these augmented types make it into projects where they don't belong and state structure would seriously become weird at that point.

That's fair. I could imagine ways augments could mess with more than you'd sign up for. Maybe something like first-party type helpers could help with that but I don't feel too strongly about this, was just an idea.

My personal approach would honestly be to replace the whole "static typing" page with a section that emphazises that we really really really want people to use RTK.

Sometimes simplicity is the best answer haha. I actually like that but at this point I'm not sure if there's too much additive work here I can contribute. It looks like all that's needed is to move the docs to markdown files and point them to RTK.

Deleting the current docs kinda solves everything haha

@markerikson
Copy link
Contributor Author

I noted the other day that the "Usage with TS" page is actually one of the top 10 pages hits-wise over the last few months:

https://twitter.com/acemarke/status/1372591748367720451

so there's definitely value in keeping it, and also in pointing people to how we want them to use RTK as a default approach.

Given that, I'm going to go ahead and try updating the TS page myself right now.

@ricokahler , I really appreciate your enthusiasm and interest in helping out. Are there any other open issues or tasks you'd be interested in tackling? I'd definitely still love to have you involved here.

@markerikson
Copy link
Contributor Author

Fixed by #4042 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants