-
-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Changes from all commits
7ac5ed4
351a5cf
05a7a79
2140744
e961d23
7f5756b
ab3b6a2
de89335
8f4cd39
14cf3e6
aa3fbe3
0badcde
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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 |
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
import { ActionCreator, ActionCreatorsMapObject, AnyAction, Dispatch } from "redux" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I read somewhere that There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That was my initial approach but it causes a compilation error on By splitting the code path using I could inline the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe 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 I didn't think the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.