Converts a few hooks & helpers to Typescript #1743
Conversation
|
✔️ Deploy Preview for react-redux-docs ready! 🔨 Explore the source changes: 0badcde 🔍 Inspect the deploy log: https://app.netlify.com/sites/react-redux-docs/deploys/60d766f51989be0008eb3b11 😎 Browse the preview: https://deploy-preview-1743--react-redux-docs.netlify.app |
| @@ -14,7 +14,7 @@ export function createStoreHook(context = ReactReduxContext) { | |||
| ? useDefaultReduxContext | |||
| : () => useContext(context) | |||
| return function useStore() { | |||
| const { store } = useReduxContext() | |||
| const { store } = useReduxContext()! | |||
There was a problem hiding this comment.
Since the context is of type ReactReduxContextValue | null, we can't just destructure this. To keep the existing API intact we can override the compiler here and preserve existing behaviour.
Keen to hear your thoughts on whether this can/should be changed though.
There was a problem hiding this comment.
Fine by me. It's theoretically possible_ for this to fail, but in practice it shouldn't, and it should be a runtime error if there's no store. Making the assumption that it exists is the best option here.
| @@ -0,0 +1,25 @@ | |||
| import { ActionCreator, ActionCreatorsMapObject, AnyAction, Dispatch } from "redux" | |||
There was a problem hiding this comment.
I read somewhere that bindActionCreators was previously copied from redux itself so I've done the same thing to make it TS-compatible: https://github.com/reduxjs/redux/blob/master/src/bindActionCreators.ts
There was a problem hiding this comment.
Yep, we inlined this into the React-Redux codebase in one of the last couple patch releases.
There was a problem hiding this comment.
I think we can split the difference and just do a TS-ified version of the plain JS file we had in here. I don't think we need the separate single-function version for our use case here.
There was a problem hiding this comment.
That was my initial approach but it causes a compilation error on const actionCreator = actionCreators[key].
By splitting the code path using if (typeof actionCreators === 'function'), actionCreators is understood to only be ActionCreatorsMapObject<any> from that point onwards. Without the split, it is ActionCreator<any> | ActionCreatorsMapObject<any> which does not support the indexer.
I could inline the bindActionCreator() function but figured it would be easier for future reference if the code is more of a straight-up copy. Happy to change it inline (or to some other solution), whichever you prefer.
There was a problem hiding this comment.
@markerikson Not sure if you missed the above message ^ Do you have any thoughts on the path to take here?
There was a problem hiding this comment.
Yep, missed it :)
Let's just go with the copy-pasted version for now, then, and we can always re-tweak it later.
| @@ -2,7 +2,7 @@ | |||
| * @param {any} obj The object to inspect. | |||
| * @returns {boolean} True if the argument appears to be a plain object. | |||
| */ | |||
| export default function isPlainObject(obj) { | |||
| export default function isPlainObject(obj: unknown) { | |||
There was a problem hiding this comment.
A bunch of these functions are about comparing arbitrary objects so I've default to unknown everywhere. If the code made assumptions (such as using an indexer) then I changed it to use any instead.
I didn't think the FixTypeLater type was relevant here since there will never be a concrete type to change it to, though correct me if I'm wrong.
There was a problem hiding this comment.
I'm inclined to agree. We don't know what the type is at this point, and from the perspective of the code it doesn't really matter either.
| @@ -0,0 +1,25 @@ | |||
| import { ActionCreator, ActionCreatorsMapObject, AnyAction, Dispatch } from "redux" | |||
There was a problem hiding this comment.
I think we can split the difference and just do a TS-ified version of the plain JS file we had in here. I don't think we need the separate single-function version for our use case here.
|
if we stick with the current |
Not from my end, all good to go 👍 |
|
Sweet, thanks! |
Following on #1742 this converts a few more files. The idea is to do the low-hanging fruit first (these were all relatively painless) to build up type coverage in the code base, making it easier to take on the big meaty source files.