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

adding types order, moved types to package #52

Merged
merged 11 commits into from
Dec 2, 2021
Merged

adding types order, moved types to package #52

merged 11 commits into from
Dec 2, 2021

Conversation

danielart
Copy link
Collaborator

@danielart danielart commented Nov 25, 2021

What is the purpose of this pull request? (put an "X" next to item)

[ ] Documentation update
[ ] Bug fix
[ ] New feature
[x ] Other, please explain: Add types d.ts

What changes did you make? (Give an overview)
add types based on proposition at issue #23
Which issue (if any) does this pull request address?

Is there anything you'd like reviewers to focus on?
types, test it, edge cases

@danielart danielart self-assigned this Nov 25, 2021
@danielart danielart marked this pull request as draft November 25, 2021 20:53
package.json Show resolved Hide resolved
@danielart
Copy link
Collaborator Author

created canary release to test this MR -> 0.9.0-canary.2


export type Hoc<S> = { store: HookReturn<S> };

type HocFunc<S, R extends React.ComponentClass = React.ComponentClass> = (

Choose a reason for hiding this comment

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

wondering if we should type the R as
R extends React.ComponentType
as the ComponentType would cover both React.FunctionComponent<> | React.ClassComponent<>

Copy link
Collaborator

Choose a reason for hiding this comment

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

Our advice is to use the useStore hook rather than the withStore HOC whenever it is a functional component. So I don't think it's wrong to have only the React.ClassComponent. In which cases would it make sense to use the withStore in a functional component?

If there is some sense that we didn't take into account then I think it's okay to change it. 🤔

@Chetan-unbounce
Copy link

minor comments but rest looks good

package.json Outdated Show resolved Hide resolved
@danielart danielart marked this pull request as ready for review December 1, 2021 18:52
@aralroca aralroca merged commit b70f074 into master Dec 2, 2021
@aralroca aralroca deleted the add-ts-types branch December 2, 2021 15:24
@aralroca
Copy link
Collaborator

aralroca commented Dec 2, 2021

@all-contributors please add @danielart for code

@allcontributors
Copy link
Contributor

@aralroca

@danielart already contributed before to code

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.

3 participants