-
Notifications
You must be signed in to change notification settings - Fork 2
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
Feature/stores #44
Merged
Merged
Feature/stores #44
Changes from 5 commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
55c202e
update dependencies
jesusrodrz 07af1ad
feature stores
jesusrodrz 63057d3
bump version
jesusrodrz d6ed659
fix action
jesusrodrz 4190d87
improve peer deps
jesusrodrz a181236
refacto pub sub pattern to avoid null value
jesusrodrz 9374b3b
remane hooks
jesusrodrz 4e2367b
improve store hooks
jesusrodrz b4cd4b4
add new hook
jesusrodrz File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,56 @@ | ||
import { useEffect, useRef, useState } from 'react'; | ||
import { Store } from './store'; | ||
|
||
/** | ||
* @param {Store} store - Store to subscribe. | ||
* @param {Function} callback - Function to call on each dipatch. | ||
* @param {Function} errorcCallback - Function to call on each error dipatch. | ||
*/ | ||
export function useStoreSub<T>( | ||
alacret marked this conversation as resolved.
Show resolved
Hide resolved
|
||
store: Store<T>, | ||
callback: ((data: T) => void) | undefined = undefined, | ||
errorcCallback: ((data: Error) => void) | undefined = undefined, | ||
): void { | ||
const callbacksRef = useRef({ | ||
callback, | ||
errorcCallback, | ||
}); | ||
|
||
callbacksRef.current = { | ||
callback, | ||
errorcCallback, | ||
}; | ||
|
||
useEffect(() => { | ||
const unsubscribeSuccess = store.subscribe((data) => { | ||
if (callbacksRef.current.callback) { | ||
callbacksRef.current.callback(data); | ||
} | ||
}); | ||
|
||
const unsubscribeError = store.subscribeError((data) => { | ||
if (callbacksRef.current.errorcCallback) { | ||
callbacksRef.current.errorcCallback(data); | ||
} | ||
}); | ||
|
||
return () => { | ||
unsubscribeSuccess.unsubscribe(); | ||
unsubscribeError.unsubscribe(); | ||
}; | ||
}, [store]); | ||
} | ||
|
||
/** | ||
* @param {Store} store - Store to subscribe. | ||
* @returns {Object} - Resulto object from the store. | ||
*/ | ||
export function useStore<T>(store: Store<T>): T { | ||
const [state, setState] = useState(store.get()); | ||
|
||
useStoreSub(store, (data) => { | ||
setState(data); | ||
}); | ||
|
||
return state; | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,34 @@ | ||
import { CheckDispatchType, Store } from './store'; | ||
|
||
/** | ||
* @param {Store} store - Event. | ||
* @param {Function} callback - Callback. | ||
* @param {Function} sideEffect - Callback. | ||
* @returns {Function} Reducer fucntion. | ||
*/ | ||
export function createStoreAction<T, V extends unknown[], U = unknown>( | ||
store: Store<T, U>, | ||
callback: | ||
| ((prevState: T, ...params: V) => CheckDispatchType<T, U>) | ||
| ((prevState: T, ...params: V) => Promise<CheckDispatchType<T, U>>), | ||
sideEffect?: (...params: V) => void, | ||
) { | ||
return (...params: V): void => { | ||
if (sideEffect) { | ||
sideEffect(...params); | ||
} | ||
const result = callback(store.get(), ...params); | ||
|
||
if (result instanceof Promise) { | ||
result | ||
.then((data) => { | ||
store.dispatch(data); | ||
}) | ||
.catch((e) => { | ||
store.dispatchError(e); | ||
}); | ||
return; | ||
} | ||
store.dispatch(result); | ||
}; | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
I think we can use
Subscriber<T>
and we can remove the| null
typingThere 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.
@alacret Done
I've crea two hooks
useStoreSubcription
anduseStoreErrorSubscription
that handle both scenarios separatelyThere 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.
if we want to manage both scenarios separately then what's the benefit of using a store instead of 2 events? or what's the difference?
My guess is that we want to reduce the boilerplate. That's why I'm suggesting a single hook. Maybe both functions can be optional
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.
Having both is better for out internal composability and performance.
for example the
useStore
hook useuseStoreSubcription
internally if we only have oneuseSubscription
the useStore hook would be inheriting the logic for the errors but it won't need it.So yes, it have more boilerplate but it is only internally the hooks that we will do not have it
Also i want you to keep in mind that the way i approaching this implementation is step by step, because we don't want to introduce breaking changes and have a lot of work to keep the leagacy features neither
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.
@alacret
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.
@jesusrodrz we need at least a second opinion here.
You say:
Having both is better for our internal composability and performance.
But this is what we already have with the
Events API
so having it separately not sure what's the benefit of that over the currentEvents API
Also, we have to keep in mind always:
We have to aim for a unique API, we don't want to have 2 ways of doing the same thing. So this
Store API
has to be thought to replace theEvents API
if and only if we can make it better.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.
Doing the hook separately, 99% or our use cases looks like this:
Which looks pretty much the same
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.
Yes, but this the first interations, with the next features like the other hooks i will be more usefull and simpler.
But i don't wanna to have a huge pull request, let move on step by step.
And that the pattern that i want to avoid, it is very imperative. the idea is that we dont have to use the useSubscription in most of the cases.
An replaced with a new hooks api that we only hace to declare one store, and s cople of actions