Skip to content

Converts a few hooks & helpers to Typescript #1743

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

Merged
merged 12 commits into from
Jun 29, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
File renamed without changes.
File renamed without changes.
2 changes: 1 addition & 1 deletion src/hooks/useStore.js → src/hooks/useStore.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ export function createStoreHook(context = ReactReduxContext) {
? useDefaultReduxContext
: () => useContext(context)
return function useStore() {
const { store } = useReduxContext()
const { store } = useReduxContext()!
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

return store
}
}
Expand Down
4 changes: 2 additions & 2 deletions src/utils/batch.js → src/utils/batch.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
// Default to a dummy "batch" implementation that just runs the callback
function defaultNoopBatch(callback) {
function defaultNoopBatch(callback: () => void) {
callback()
}

let batch = defaultNoopBatch

// Allow injecting another batching function later
export const setBatch = (newBatch) => (batch = newBatch)
export const setBatch = (newBatch: (callback: () => void) => void) => (batch = newBatch)

// Supply a getter just to skip dealing with ESM bindings
export const getBatch = () => batch
10 changes: 0 additions & 10 deletions src/utils/bindActionCreators.js

This file was deleted.

25 changes: 25 additions & 0 deletions src/utils/bindActionCreators.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
import { ActionCreator, ActionCreatorsMapObject, AnyAction, Dispatch } from "redux"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, we inlined this into the React-Redux codebase in one of the last couple patch releases.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@markerikson Not sure if you missed the above message ^ Do you have any thoughts on the path to take here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, missed it :)

Let's just go with the copy-pasted version for now, then, and we can always re-tweak it later.


function bindActionCreator<A extends AnyAction = AnyAction>(
actionCreator: ActionCreator<A>,
dispatch: Dispatch
) {
return function (this: any, ...args: any[]) {
return dispatch(actionCreator.apply(this, args))
}
}

export default function bindActionCreators(actionCreators: ActionCreator<any> | ActionCreatorsMapObject, dispatch: Dispatch) {
if (typeof actionCreators === 'function') {
return bindActionCreator(actionCreators, dispatch)
}

const boundActionCreators: ActionCreatorsMapObject = {}
for (const key in actionCreators) {
const actionCreator = actionCreators[key]
if (typeof actionCreator === 'function') {
boundActionCreators[key] = (...args) => dispatch(actionCreator(...args))
}
}
return boundActionCreators
}
2 changes: 1 addition & 1 deletion src/utils/isPlainObject.js → src/utils/isPlainObject.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

if (typeof obj !== 'object' || obj === null) return false

let proto = Object.getPrototypeOf(obj)
Expand Down
4 changes: 2 additions & 2 deletions src/utils/shallowEqual.js → src/utils/shallowEqual.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
function is(x, y) {
function is(x: unknown, y: unknown) {
if (x === y) {
return x !== 0 || y !== 0 || 1 / x === 1 / y
} else {
return x !== x && y !== y
}
}

export default function shallowEqual(objA, objB) {
export default function shallowEqual(objA: any, objB: any) {
if (is(objA, objB)) return true

if (
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import isPlainObject from './isPlainObject'
import warning from './warning'

export default function verifyPlainObject(value, displayName, methodName) {
export default function verifyPlainObject(value: unknown, displayName: string, methodName: string) {
if (!isPlainObject(value)) {
warning(
`${methodName}() in ${displayName} must return a plain object. Instead received ${value}.`
Expand Down
2 changes: 1 addition & 1 deletion src/utils/warning.js → src/utils/warning.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
* @param {String} message The warning message.
* @returns {void}
*/
export default function warning(message) {
export default function warning(message: string) {
/* eslint-disable no-console */
if (typeof console !== 'undefined' && typeof console.error === 'function') {
console.error(message)
Expand Down