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

[DRAFT]: Docs for usage with typescript #3201

Merged
merged 38 commits into from Jan 10, 2019
Merged

[DRAFT]: Docs for usage with typescript #3201

merged 38 commits into from Jan 10, 2019

Conversation

ghost
Copy link

@ghost ghost commented Oct 29, 2018

Status

[January 9, 2019] - Fourth draft completed
[November 12, 2018] - Third draft completed, next revision will include usage with React-Redux & Thunks
[November 10, 2018] - Second draft completed, looking for feedback
[October 29, 2018] - Initial draft ready, looking for feedback

Description

From #2955 , this adds documentation around using redux with typescript.

Notes & Considerations

  1. I chose to do the bulk of the documentation here around the example of a basic chat application. This allows for the examples to be more practical and interactive (codesandbox).
  2. The docs include a codesandbox example for people to explore. If anyone feels something here could be improved on please let me know. Here's the link.
  3. The implementation of type checking varies slightly between different blog posts. I chose an approach that I consider reasonable. I'm open to suggestions here.

Misc.

  1. I placed the new doc under the "Advanced" section. If there is a more appropriate place for this specific doc I'd be happy to move it as needed.

  2. I haven't linked this doc to the table of contents (README). If I should do that please let me know.

@markerikson
Copy link
Contributor

I'll try to take a look at this in the next few days - been busy with ReactConf and work.

Copy link

@Jessidhia Jessidhia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find the use of I in interface names...... icky. It smells of C++ and C# interfaces which are completely different from structural interfaces.

docs/advanced/UsageWithTypescript.md Outdated Show resolved Hide resolved
docs/advanced/UsageWithTypescript.md Outdated Show resolved Hide resolved
docs/advanced/UsageWithTypescript.md Show resolved Hide resolved
@Cryrivers
Copy link
Contributor

Saw @markerikson 's tweet and made a TypeScript + Redux example based our practice. Check it out: https://codesandbox.io/s/62kw7xvnqw (Note: Do download and try it in VSCode. The type inference on CodeSandbox is not working perfectly at the moment.)

To List some advantages:

  1. No need to describe the shape/type of actions and action creators
  2. Automatically map type of reducers and actions to props using helper WithRedux
  3. Type-safe actions (see screenshot).

image

@ghost
Copy link
Author

ghost commented Nov 8, 2018

Thank you for the feedback so far. Before I work on the necessary changes, some points of discussion:

@Kovensky

  1. There is an optional TSLint rule that forces interface names to start with an "I" so wanted feedback here. Looks like this convention is not that common so I will change it.
  2. Regarding hoisted functions, this is fine and I will change it accordingly.
  3. Thanks for catching the lack of type checking for reducers. There is a slightly different method for type checking reducers by @Cryrivers . I would like to see what people think before making the appropriate changes.

@CarloPalinckx

  1. There are multiple popular naming conventions for actions. The current names for the actions might be confusing because of the very basic demonstration. For example, in addition to @CHAT:SEND/MESSAGE, there could have also been @CHAT:SEND/FILE. I think the assumption here is that people reading this section of the docs would already be familiar with redux, meaning they would have an opinion on which convention to use. I'm still willing to change it if you think it will add clarity. If we do end up changing to SEND_MESSAGE I would probably also change @SYSTEM:UPDATE/SESSION to UPDATE_SESSION for consistency.

@CarloPalinckx
Copy link

CarloPalinckx commented Nov 8, 2018

@HershVar It seems like all the pages in the docs stay away from this particular convention so I wouldn't introduce that in docs about typescript. I also don't think new people will be very familiar with redux at this stage other than have read the previous pages. If you are integrating redux straight into a typescript environment, you would have to have read these docs in order to get started.

@ghost
Copy link
Author

ghost commented Nov 8, 2018

@CarloPalinckx
Good point, I will make those changes in order to stay consistent with the rest of the documentation as you pointed out.

Copy link

@punmechanic punmechanic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

docs/advanced/UsageWithTypescript.md Outdated Show resolved Hide resolved
docs/advanced/UsageWithTypescript.md Outdated Show resolved Hide resolved
docs/advanced/UsageWithTypescript.md Outdated Show resolved Hide resolved
docs/advanced/UsageWithTypescript.md Outdated Show resolved Hide resolved
docs/advanced/UsageWithTypescript.md Show resolved Hide resolved
docs/advanced/UsageWithTypescript.md Outdated Show resolved Hide resolved
docs/advanced/UsageWithTypescript.md Outdated Show resolved Hide resolved
@ghost
Copy link
Author

ghost commented Nov 10, 2018

@danpantry
Thank you for this feedback, in combination with the other responses here I will make the appropriate changes this weekend. Will cc when the update is pushed.

@CarloPalinckx
Copy link

Have you considered adding an "advanced" section where you go over things like inference? It currently feels a bit weird you are only using the return type of the root reducer and not the return types for action creators and chat/system reducers. You could stay away from inference at all in the "regular" part and then go all in on inference in that section. I feel like the "Note that you can use inference here" could be relevant to more places in the docs, but that would get pretty messy imo.

@Cryrivers
Copy link
Contributor

@CarloPalinckx This is also my point from beginning. You can check out my proposed solution, which utilises type inference as much as possible.

#3201 (comment)

@ghost
Copy link
Author

ghost commented Nov 23, 2018

@CarloPalinckx
Thanks for the feedback, there's definitely multiple ways to add type checking with varying levels of granularity. I'm not 100% sure on how opinionated we should be and what the opinion should be (level of granularity used in type checking). That being said, I did mention as a note in the end that the example uses just one of many possible ways to type check. If there are specific suggestions we can go from there.

Regarding the addition of "usage with redux thunk" there is one thing I am trying to figure out right now. A thunk returns a function but calling the thunk via props in a component will either return nothing or some other value that is explicitly returned. If anyone has any recommendation on how to add type checking here I would love to hear suggestions. I think the goal here is to have type checking around the parameters and what calling that thunk will return.

@markerikson
Copy link
Contributor

So where do we think this PR stands now?

@ghost
Copy link
Author

ghost commented Dec 17, 2018

I think there are 2 points of discussion to resolve before considering this "ready".

  1. Usage of enums; @danpantry pointed out an alternative to get rid of enums altogether. Out in the wild, based on my preliminary research, there doesn't seem to be a dominant preference for either method.

  2. There seems to be two emerging ways to use typescript here; Using typescript as explicitly as possible or relying on typescript's inference. Right now this PR uses bits and pieces from both styles, some parts are explicit and some parts use inference. It is mentioned at the bottom of this docs' page that there are multiple ways to implement typescript but should we be leaning to one side here? Some time has passed since this specific discussion started so opinions may have changed.

@markerikson
Copy link
Contributor

Hmm. How difficult would it be to show kind of "side-by-side" examples of both approaches? What are the tradeoffs of each?

@ghost
Copy link
Author

ghost commented Dec 19, 2018

I think doing that may add some unnecessary noise to this page. Going down that path feels like we are teaching more about typescript instead of focusing on the main goal which is to teach how to use redux with typescript.

@joshburgess
Copy link
Contributor

joshburgess commented Dec 29, 2018

@HershVar RE: enums vs string literals

People use both. The unique thing about enums here is that the values exist at both run-time and compile-time, but you can accomplish exactly the same thing with constant string literals. It just requires a little more boilerplate.

// the assertions here aren't necessary... just showing what gets inferred
const ACTION_A: 'ACTION_A' = 'ACTION_A'
const ACTION_B: 'ACTION_B' = 'ACTION_B'

export const ActionTypesMap = {
  ACTION_A,
  ACTION_B,
}
export type ActionTypesMap = typeof ActionTypesMap

export ActionTypes = ACTION_A | ACTION_B

is almost the same thing as

enum ActionTypes {
  ACTION_A = 'ACTION_A',
  ACTION_B = 'ACTION_B'
}

The only difference is that the former's individual types can't be accessed with dot notation, like ActionTypesMap.ACTION_A. You, instead, have to do ActionTypesMap['ACTION_A']. You could replicate the dot notation functionality here if you used TypeScript's namespaces feature though... but not many people use it. (It's one of the unsupported features of TS when using Babel 7, as far as I know).

Personally, I prefer constants, even though they require extra boilerplate. Why? Mostly just because enums really only provide an alternate syntax for an already existing language feature: discriminated unions.

@punmechanic
Copy link

With regards to:

Going down that path feels like we are teaching more about typescript instead of focusing on the main goal which is to teach how to use redux with typescript.

If that's the case - Just go with string literals. Enums are unique to TypeScript and I have never personally seen them used for this use case nor would I recommend them. Every JavaScript and TypeScript user is going to understand what a string literal is.

@ghost
Copy link
Author

ghost commented Jan 3, 2019

@joshburgess @danpantry

Thanks, I'll make the appropriate changes as soon as I get the time this week. My schedule will finally free up starting next week so I plan to finish up this PR very soon.

@JounQin
Copy link

JounQin commented Jan 6, 2019

Is that possible to use thunk actions without casting thunkSendMessage to any of AppProps in the example?

@ghost
Copy link
Author

ghost commented Jan 8, 2019

@JounQin
Not sure if I am understanding your question but:

  1. From what I understand the getState param in redux thunk returns a method to get the entire state of the store. In that case we would have to use AppState type to get correct type checking and intelisense.

  2. If you are referring to Action<string> as the action type in thunks.ts, we are still getting type checking because we are using action creators whenever we dispatch.

Hopefully this answered your question but feel free to clarify.

@netlify
Copy link

netlify bot commented Jan 10, 2019

Deploy preview for redux-docs ready!

Built with commit d930250

https://deploy-preview-3201--redux-docs.netlify.com

@Jessidhia
Copy link

Jessidhia commented Jan 10, 2019

Very importantly -- this only works if you use const to declare the key (no var or let), and the initializer must be a string literal (template literals acceptable only if they are untagged and have no substitutions).

I kinda prefer the enum approach as it lets you use CamelCase instead of SHOUTING_CASE without worrying about naming collisions, but it's true that using exported consts will be much better for bundle size as the identifier names can be minified; enum key names cannot.


You can make the type of the consts easier to use by also exporting an associated type (export const FOO = 'FOO'; export type FOO = typeof FOO;), which has an effect similar to how enum keys are their own type. I don't necessarily recommend this, though, it just makes it easier to use "FOO"'s type without having to know to write "typeof FOO".

The thing I think is a problem, though, is that you no longer get an automatic type with all const values.

For that, you need something like export type Actions = Extract<typeof import('./types')[keyof typeof import('./types')], string>, but that will not compile with CRA because babel's parser doesn't support import() types yet. The version that works with CRA is:

// you put this in types.ts itself, it's a circular import but it's
// only used for its typeof, not for its value, so it's safe
import * as types from './types'

export type Actions = Extract<typeof types[keyof typeof types], string>

The Extract itself is only needed if you export any other concrete values from types.ts. If your only value exports are already strings you can use just the typeof expression.

You also also have to make sure none of your exports are actually typed string, otherwise Actions will collapse to string; and you need to be careful when reexporting Actions as it cannot be directly reexported (it can be with tsc / ts-loader, but it'll break with CRA / babel or with --isolatedModules). To reexport it (from, say, index.ts), you need to alias it:

import { Actions } from './types'
export type Actions = Actions

The above are all things that just using an enum will do for you. So it's all about trade-offs...

@nurpax
Copy link

nurpax commented Jan 10, 2019

How important is using

export const FOO = ’FOO’

for action types in the first place! They’re only used in reducers and action creators. I find that this pattern used to be needed on normal JS to prevent misspelling the stringly typed action names. But on TS, if your reducers are written with full static types, typoing say ’FOO’ to ’Foo’ will result in an error.

So I’ve moved away from declaring const action type symbols in most of my new code.

@Jessidhia
Copy link

Jessidhia commented Jan 10, 2019

You still need this to, ergonomically, ensure the literal string becomes a literal type if you don't have a contextual type. If you write the string inline in an object (say, { type: 'FOO' }), it will actually become of type { type: string } unless you write { type: 'FOO' as 'FOO' }.

But true, it is possible to write it without requiring the consts by always using interfaces that provide a literal contextual type.

export interface FooAction {
  type: 'FOO'
}

export function doFoo(): FooAction {
  // has a contextual type from the return annotation
  // so 'FOO' will be literally typed
  return { type: 'FOO' }
}

export default function(state = {}, action: FooAction) {
  switch(action.type) {
    case 'FOO':
      // perform 'FOO'
      break
    default:
      unreachable(action)
  }
  return state
}

// trick to get the compiler to check for exhaustiveness in the switch
function unreachable(value: never) {}

The downside is when you want to aggregate the types that you accept. You can no longer use that typeof trick that I demonstrated and have to manually | all your interfaces and index their ['type'] key (you have to use bracket accesses in type space; only modules and enums can use dots).

@timdorr
Copy link
Member

timdorr commented Jan 10, 2019

OK, this has been open way way too long and looks pretty good to me. I'm going to merge it in. It won't get automatically added to the site, since it's just a single new file. But this opens the door for others to provide PRs on this file and critique via changes, instead of comments.

Thanks a ton @HershVar for sticking with this!

@timdorr timdorr merged commit de1f45a into reduxjs:master Jan 10, 2019
@markerikson
Copy link
Contributor

markerikson commented Jan 12, 2019

I know this PR got merged already, but just saw an interesting-looking post and wanted to use this as a venue to ping people. Does this post offer any additional useful techniques that are worth showing?

https://medium.com/@ankeet.maini/type-redux-with-typescript-without-writing-any-types-5f96cbfef806

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.

10 participants